Skip to content
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

fix: improved currency detection #1345

Merged
merged 7 commits into from
Feb 5, 2024
Merged

fix: improved currency detection #1345

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8a6683f
improvements to currency detection
KolevDarko Jan 25, 2024
267f55a
test rename
KolevDarko Jan 25, 2024
bb7c4c3
Merge branch 'master' into fix/currency-fromid-precedence
KolevDarko Jan 25, 2024
b16b45d
Update packages/currency/src/erc20/index.ts
KolevDarko Jan 26, 2024
9062de8
Merge branch 'master' into fix/currency-fromid-precedence
KolevDarko Feb 2, 2024
906dfba
fixes from PR comments
KolevDarko Feb 2, 2024
8fc17d0
Merge branch 'master' into fix/currency-fromid-precedence
KolevDarko Feb 2, 2024
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
14 changes: 9 additions & 5 deletions packages/currency/src/currencyManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,17 +71,21 @@ export class CurrencyManager<TMeta = unknown> implements ICurrencyManager<TMeta>
return this.fromAddress(currencyIdentifier, network);
}

if (network && currencyIdentifier.indexOf(network) === -1) {
currencyIdentifier = `${currencyIdentifier}-${network}`;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In making the improvement below, we need to handle the case where we provide the network as separate parameter but the network is not within the symbol.
There is an issue when we have both Fiat and ERC20 token with same symbol, like MNT.
MNT - fiat currency
MNT-mainnet - Ethereum token.
The simplest fix here was to add the network to the currencyIdentifier if it's not already there.


const currencyFromId = this.fromId(currencyIdentifier);

if (currencyFromId) return currencyFromId;
Comment on lines +79 to +81
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get this. If you are using this method by passing always IDs, why not use fromId directly?

Copy link
Member

Choose a reason for hiding this comment

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

You're right, we should always use fromId in user code when we are sure to have an ID as input.

In the case of from, we're not entirely sure what kind of input we have (ID, symbol, address). In this case, we want to prioritize looking by ID rather than symbol because it's more precise (like we already prioritize looking by address first). More context here: #1345 (comment)

I'm sure we are over-using from sometimes where we could replace it with fromId in user code, but the path with from should work too and needs to be fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

should we deprecate from?

Copy link
Member

@alexandre-abrioux alexandre-abrioux Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that could be a good start to add the @deprecated JSDOC 👍


Comment on lines +79 to +82
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Main purpose of this PR is to prioritize the method CurrencyManager.fromId before fromSymbol, because it matches the direct id of the currency, it's more specific.

const parts = currencyIdentifier.split('-');
const currencyFromSymbol =
this.fromSymbol(parts[0], network || (parts[1] as CurrencyTypes.ChainName)) ||
// try without splitting the symbol to support currencies like ETH-rinkeby
this.fromSymbol(currencyIdentifier, network);

if (currencyFromSymbol) {
return currencyFromSymbol;
}

return this.fromId(currencyIdentifier);
return currencyFromSymbol;
}

/**
Expand Down
1 change: 1 addition & 0 deletions packages/currency/src/erc20/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export function getSupportedERC20Tokens(): ERC20Currency[] {
network: networkName,
decimals: token.decimals,
symbol: token.symbol,
id: token.id || `${token.symbol}-${networkName}`,
})),
];
},
Expand Down
2 changes: 1 addition & 1 deletion packages/currency/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { CurrencyTypes, RequestLogicTypes } from '@requestnetwork/types';
* Common types used in token configuration files
*/
type TokenAddress = string;
type TokenDefinition = { name: string; symbol: string; decimals: number };
type TokenDefinition = { name: string; symbol: string; decimals: number; id?: string };
export type TokenMap = Record<TokenAddress, TokenDefinition>;

/**
Expand Down
42 changes: 42 additions & 0 deletions packages/currency/test/currencyManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,4 +709,46 @@ describe('CurrencyManager', () => {
expect(path).toMatchObject([eur.hash, dai.hash.toLowerCase()]);
});
});

describe('Native and bridged USDC', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The describe and it should give details on the behaviour you are testing, not about the data.

const USDC_LIST = [
{
address: '0x2791Bca1f2de4661ED88A30C99A7a9449Aa84174',
name: 'USD Coin (PoS)',
symbol: 'USDCe',
network: 'matic',
id: 'USDC-matic',
Copy link
Contributor

Choose a reason for hiding this comment

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

can you give context why the ID wouldn't be USDCe-matic for this token?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @benjlevesque; maybe a small comment in the code explaining why we test a currency with an ID and a Symbol that differs would do. Here is some context about it, @KolevDarko feel free to reuse some of the explanation for the comment in the code: #1345 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an elaborate comment above the describe block; there are many ways to explain this, I'm open to suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good!

decimals: 6,
type: RequestLogicTypes.CURRENCY.ERC20,
} as CurrencyInput,
{
address: '0x3c499c542cEF5E3811e1192ce70d8cC03d5c3359',
name: 'USD Coin',
symbol: 'USDC',
network: 'matic',
id: 'USDCn-matic',
decimals: 6,
type: RequestLogicTypes.CURRENCY.ERC20,
} as CurrencyInput,
];
const currencyManager = new CurrencyManager(USDC_LIST);
it('USDCe matic', () => {
expect(currencyManager.from('USDC-matic')).toMatchObject({
type: RequestLogicTypes.CURRENCY.ERC20,
network: 'matic',
symbol: 'USDCe',
id: 'USDC-matic',
address: '0x2791Bca1f2de4661ED88A30C99A7a9449Aa84174',
});
});
it('native USDC matic', () => {
expect(currencyManager.from('USDCn-matic')).toMatchObject({
type: RequestLogicTypes.CURRENCY.ERC20,
network: 'matic',
symbol: 'USDC',
id: 'USDCn-matic',
address: '0x3c499c542cEF5E3811e1192ce70d8cC03d5c3359',
});
});
});
});