-
Notifications
You must be signed in to change notification settings - Fork 83
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.
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
Changes from all commits
8a6683f
267f55a
bb7c4c3
b16b45d
9062de8
906dfba
8fc17d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,7 @@ export class CurrencyManager<TMeta = unknown> implements ICurrencyManager<TMeta> | |
* | ||
* @param currencyIdentifier e.g. 'DAI', 'FAU', 'FAU-rinkeby', 'ETH-rinkeby-rinkeby' or '0xFab46E002BbF0b4509813474841E0716E6730136' | ||
* @param network e.g. rinkeby, mainnet | ||
* @deprecated Use fromSymbol, fromAddress, fromId or fromHash to avoid ambiguity | ||
*/ | ||
from( | ||
currencyIdentifier: string | undefined, | ||
|
@@ -71,17 +72,21 @@ export class CurrencyManager<TMeta = unknown> implements ICurrencyManager<TMeta> | |
return this.fromAddress(currencyIdentifier, network); | ||
} | ||
|
||
if (network && currencyIdentifier.indexOf(network) === -1) { | ||
currencyIdentifier = CurrencyManager.currencyId({ symbol: currencyIdentifier, network }); | ||
} | ||
|
||
const currencyFromId = this.fromId(currencyIdentifier); | ||
|
||
if (currencyFromId) return currencyFromId; | ||
|
||
Comment on lines
+79
to
+82
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
/** | ||
|
@@ -218,7 +223,7 @@ export class CurrencyManager<TMeta = unknown> implements ICurrencyManager<TMeta> | |
/** | ||
* Utility function to compute the unique identifier | ||
*/ | ||
static currencyId(currency: CurrencyInput): string { | ||
static currencyId(currency: { symbol: string; network?: string }): string { | ||
return 'network' in currency ? `${currency.symbol}-${currency.network}` : currency.symbol; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -709,4 +709,54 @@ describe('CurrencyManager', () => { | |
expect(path).toMatchObject([eur.hash, dai.hash.toLowerCase()]); | ||
}); | ||
}); | ||
|
||
/** | ||
* In order to respond to the recent changes in the USDC token where | ||
* Circle introduced native USDC on chains Arbitrum, Polygon and Optimism, and renamed the bridged USDC to USDCe. | ||
* We wanted to support both the native USDC and the bridged USDCe on the same network, | ||
* while staying backwards compatible. | ||
* That is why we kept the original USDC-network id for the bridged USDC, while changing the symbol to USDCe. | ||
* Then we added the new native USDC with symbol USDC but id of USDCn-network. | ||
*/ | ||
describe('Currency Manager correctly detects native USDC and bridged USDCe on polygon', () => { | ||
const USDC_LIST = [ | ||
{ | ||
address: '0x2791Bca1f2de4661ED88A30C99A7a9449Aa84174', | ||
name: 'USD Coin (PoS)', | ||
symbol: 'USDCe', | ||
network: 'matic', | ||
id: 'USDC-matic', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you give context why the ID wouldn't be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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('Detects USDCe matic by USDC-matic id', () => { | ||
expect(currencyManager.from('USDC-matic')).toMatchObject({ | ||
type: RequestLogicTypes.CURRENCY.ERC20, | ||
network: 'matic', | ||
symbol: 'USDCe', | ||
id: 'USDC-matic', | ||
address: '0x2791Bca1f2de4661ED88A30C99A7a9449Aa84174', | ||
}); | ||
}); | ||
it('Detects native USDC matic by USDCn-matic id', () => { | ||
expect(currencyManager.from('USDCn-matic')).toMatchObject({ | ||
type: RequestLogicTypes.CURRENCY.ERC20, | ||
network: 'matic', | ||
symbol: 'USDC', | ||
id: 'USDCn-matic', | ||
address: '0x3c499c542cEF5E3811e1192ce70d8cC03d5c3359', | ||
}); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 withfromId
in user code, but the path withfrom
should work too and needs to be fixed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we deprecate
from
?There was a problem hiding this comment.
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 👍