Skip to content

use asset canonical ID for key in cache #495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
acharb merged 4 commits into from
May 17, 2022
Merged

use asset canonical ID for key in cache #495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
211f112
only show rate if amount input
acharb May 12, 2022
f40c933
use canonicalID for cached assets
acharb May 16, 2022
4003c74
fix trans details as well
acharb May 16, 2022
6ca82ef
make change for pp dest asset
acharb May 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions @shared/api/helpers/getIconUrlFromIssuer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export const getIconUrlFromIssuer = async ({
try {
/* First, check our localStorage cache in Background to see if we've found this url before */
({ iconUrl } = await sendMessageToBackground({
assetCode: code,
assetCanonical: `${code}:${key}`,

@leighmcculloch leighmcculloch May 16, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the canonical form here handle the case where the code is an ASSET_TYPE_ALPHANUM12 but only contains 3 non-zero bytes?

@acharb acharb May 16, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you give an example of what you mean by only contains 3 non-zero bytes?

@Shaptic Shaptic May 16, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a reference to this subtlety in SEP-11, https://stellar.org/protocol/sep-11#alphanum4-alphanum12:

Stellar disallows assets of type ASSET_TYPE_CREDIT_ALPHANUM12 that have fewer than 5 bytes, but such assets can be represented in binary XDR, and so in txrep an AlphaNum12 value with fewer than 5 bytes in its assetCode is rendered with trailing \x00 (escaped NUL bytes) to as to make the length 5---e.g., the 12-byte asset code ABC is rendered ABC\x00\x00. Note that stellar-core disallows non ASCII bytes in AssetCode fields, so the primary use of this feature is to construct or examine invalid transaction test cases.

@leighmcculloch leighmcculloch May 16, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On re-read, I think it is not relevant because of:

Note that stellar-core disallows non ASCII bytes in AssetCode fields, so the primary use of this feature is to construct or examine invalid transaction test cases.

type: SERVICE_TYPES.GET_CACHED_ASSET_ICON,
}));
if (iconUrl) {
Expand Down Expand Up @@ -88,7 +88,7 @@ export const getIconUrlFromIssuer = async ({
iconUrl = image;
/* And also save into the cache to prevent having to do this process again */
await sendMessageToBackground({
assetCode: code,
assetCanonical: `${code}:${key}`,
iconUrl,
type: SERVICE_TYPES.CACHE_ASSET_ICON,
});
Expand Down
6 changes: 3 additions & 3 deletions @shared/api/internal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ export const getAssetIcons = async ({
} = token;
// eslint-disable-next-line no-await-in-loop
icon = await getIconUrlFromIssuer({ key, code, networkDetails });
assetIcons[code] = icon;
assetIcons[`${code}:${key}`] = icon;
}
}
}
Expand All @@ -343,15 +343,15 @@ export const retryAssetIcon = async ({
const newAssetIcons = { ...assetIcons };
try {
await sendMessageToBackground({
assetCode: code,
assetCanonical: `${code}:${key}`,
iconUrl: null,
type: SERVICE_TYPES.CACHE_ASSET_ICON,
});
} catch (e) {
return assetIcons;
}
const icon = await getIconUrlFromIssuer({ key, code, networkDetails });
newAssetIcons[code] = icon;
newAssetIcons[`${code}:${key}`] = icon;
return newAssetIcons;
};

Expand Down
1 change: 1 addition & 0 deletions @shared/api/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export interface Response {
allAccounts: Array<Account>;
accountName: string;
assetCode: string;
assetCanonical: string;
iconUrl: string;
network: string;
recentAddresses: Array<string>;
Expand Down
8 changes: 4 additions & 4 deletions extension/src/background/messageListener/popupMessageListener.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,24 +637,24 @@ export const popupMessageListener = (request: Request) => {
};

const getCachedAssetIcon = () => {
const { assetCode } = request;
const { assetCanonical } = request;

const assetIconCache = JSON.parse(
localStorage.getItem(CACHED_ASSET_ICONS_ID) || "{}",
);

return {
iconUrl: assetIconCache[assetCode] || "",
iconUrl: assetIconCache[assetCanonical] || "",
};
};

const cacheAssetIcon = () => {
const { assetCode, iconUrl } = request;
const { assetCanonical, iconUrl } = request;

const assetIconCache = JSON.parse(
localStorage.getItem(CACHED_ASSET_ICONS_ID) || "{}",
);
assetIconCache[assetCode] = iconUrl;
assetIconCache[assetCanonical] = iconUrl;
localStorage.setItem(CACHED_ASSET_ICONS_ID, JSON.stringify(assetIconCache));
};

Expand Down
14 changes: 11 additions & 3 deletions extension/src/popup/components/account/AccountAssets/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { BigNumber } from "bignumber.js";
import { AssetIcons } from "@shared/api/types";
import { retryAssetIcon } from "@shared/api/internal";

import { getCanonicalFromAsset } from "helpers/stellar";
import StellarLogo from "popup/assets/stellar-logo.png";
import { settingsNetworkDetailsSelector } from "popup/ducks/settings";

Expand All @@ -21,11 +22,15 @@ export const AssetIcon = ({
issuerKey: string;
retryAssetIconFetch?: (arg: { key: string; code: string }) => void;
}) =>
assetIcons[code] || code === "XLM" ? (
assetIcons[getCanonicalFromAsset(code, issuerKey)] || code === "XLM" ? (
<img
className="AccountAssets__asset--logo"
alt={`${code} logo`}
src={code === "XLM" ? StellarLogo : assetIcons[code] || ""}
src={
code === "XLM"
? StellarLogo
: assetIcons[getCanonicalFromAsset(code, issuerKey)] || ""
}
onError={() => {
if (retryAssetIconFetch) {
retryAssetIconFetch({ key: issuerKey, code });
Expand Down Expand Up @@ -77,7 +82,10 @@ export const AccountAssets = ({
return (
<>
{sortedBalances.map(({ token: { issuer, code }, total }) => (
<div className="AccountAssets__asset" key={code}>
<div
className="AccountAssets__asset"
key={`${code}:${issuer?.key || ""}`}
>
<div className="AccountAssets__copy-left">
<AssetIcon
assetIcons={assetIcons}
Expand Down
4 changes: 2 additions & 2 deletions extension/src/popup/components/manageAssets/ChooseAsset/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import { ROUTES } from "popup/constants/routes";
import { sortBalances } from "popup/helpers/account";
import { transactionSubmissionSelector } from "popup/ducks/transactionSubmission";
import { settingsNetworkDetailsSelector } from "popup/ducks/settings";

import { SubviewHeader } from "popup/components/SubviewHeader";
import { getCanonicalFromAsset } from "helpers/stellar";

import { Balances } from "@shared/api/types";

Expand Down Expand Up @@ -57,7 +57,7 @@ export const ChooseAsset = ({ balances, setErrorAsset }: ChooseAssetProps) => {
collection.push({
code,
issuer: issuer?.key || "",
image: assetIcons[code],
image: assetIcons[getCanonicalFromAsset(code, issuer?.key)],
domain,
});
}
Expand Down
4 changes: 2 additions & 2 deletions extension/src/popup/components/manageAssets/ManageAssetRows/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,9 @@ export const ManageAssetRows = ({
const isActionPending = submitStatus === ActionStatus.PENDING;

return (
<div className="ManageAssetRows__row" key={code}>
<div className="ManageAssetRows__row" key={canonicalAsset}>
<AssetIcon
assetIcons={code !== "XLM" ? { [code]: image } : {}}
assetIcons={code !== "XLM" ? { [canonicalAsset]: image } : {}}
code={code}
issuerKey={issuer}
/>
Expand Down
2 changes: 1 addition & 1 deletion extension/src/popup/components/sendPayment/SendAmount/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ export const SendAmount = ({ previous }: { previous: ROUTES }) => {
<div className="SendAmount__input-amount__asset-copy">
{getAssetFromCanonical(formik.values.asset).code}
</div>
{destinationAsset && (
{destinationAsset && formik.values.amount !== "0" && (

@acharb acharb May 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change for the "no path found" fix

<ConversionRate
loading={loadingRate}
source={getAssetFromCanonical(formik.values.asset).code}
Expand Down
12 changes: 9 additions & 3 deletions extension/src/popup/components/sendPayment/SendConfirm/TransactionDetails/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { Card, Loader, Icon } from "@stellar/design-system";

import {
getAssetFromCanonical,
getCanonicalFromAsset,
xlmToStroop,
getConversionRate,
truncatedFedAddress,
Expand Down Expand Up @@ -76,7 +77,9 @@ export const TransactionDetails = ({ goBack }: { goBack: () => void }) => {
code: destAsset.code,
networkDetails,
});
setDestAssetIcons({ [destAsset.code]: iconURL });
setDestAssetIcons({
[getCanonicalFromAsset(destAsset.code, destAsset.issuer)]: iconURL,
});
})();
}, [destAsset.code, destAsset.issuer, networkDetails]);

Expand Down Expand Up @@ -205,7 +208,10 @@ export const TransactionDetails = ({ goBack }: { goBack: () => void }) => {
assetIcons={assetIcons}
sortedBalances={[
{
token: { issuer: sourceAsset.issuer, code: sourceAsset.code },
token: {

@acharb acharb May 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slightly related fix, AccountBalances expects the issuer to be nested one more level under the key field, oops

issuer: { key: sourceAsset.issuer },
code: sourceAsset.code,
},
total: amount || "0",
},
]}
Expand All @@ -218,7 +224,7 @@ export const TransactionDetails = ({ goBack }: { goBack: () => void }) => {
sortedBalances={[
{
token: {
issuer: destAsset.issuer,
issuer: { key: destAsset.issuer },
code: destAsset.code,
},
total: destinationAmount || "0",
Expand Down