fix: improved currency detection #1345
Conversation
github-actions
bot
requested review from
kevindavee,
leoslr,
MantisClone and
olivier7delf
| const currencyFromId = this.fromId(currencyIdentifier); | ||
|
|
||
| if (currencyFromId) return currencyFromId; | ||
|
|
There was a problem hiding this comment.
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.
| if (network && currencyIdentifier.indexOf(network) === -1) { | ||
| currencyIdentifier = `${currencyIdentifier}-${network}`; | ||
| } |
There was a problem hiding this comment.
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.
Co-authored-by: Alexandre ABRIOUX <[email protected]>
| @@ -709,4 +709,46 @@ describe('CurrencyManager', () => { | |||
| expect(path).toMatchObject([eur.hash, dai.hash.toLowerCase()]); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('Native and bridged USDC', () => { | |||
There was a problem hiding this comment.
The describe and it should give details on the behaviour you are testing, not about the data.
| const currencyFromId = this.fromId(currencyIdentifier); | ||
|
|
||
| if (currencyFromId) return currencyFromId; |
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
should we deprecate from?
There was a problem hiding this comment.
Yes, that could be a good start to add the @deprecated JSDOC 👍
| name: 'USD Coin (PoS)', | ||
| symbol: 'USDCe', | ||
| network: 'matic', | ||
| id: 'USDC-matic', |
There was a problem hiding this comment.
can you give context why the ID wouldn't be USDCe-matic for this token?
There was a problem hiding this comment.
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.
I added an elaborate comment above the describe block; there are many ways to explain this, I'm open to suggestions.
|
Found another way to solve the underlying problem I had |
|
@KolevDarko Although you ended up fixing the behavior another way, do you still think that this PR is still the "more correct way of doing things"? I want to understand if this PR still has merit or benefit for the rest of the Request Network ecosystem. |
|
I agree with @MantisClone, I think we should keep this PR open and continue to push for this change. Retrieving a currency from the currency list with the ContextWhile maintaining an extended list of currencies at Request Finance for our day-to-day operations (https://api.request.finance/currency/) we came across the need to rename some currencies. For instance, on Arbitrum, the community first used a bridged USDC token. Then in 2023, Circle deployed their own version of USDC on Aritrum, called the "native USDC token", see https://www.circle.com/blog/arbitrum-usdc-now-available. At Request Finance, we renamed the old USDC token to USDCe, and we introduced the new native token as USDC. Behind the scenes, the ID of the legacy token did not change to prevent breaking previously created Requests. So the symbol and ID of these currencies started to drift apart:
Anywhere we retrieve a currency with the |
There was a problem hiding this comment.
I'll let you answer @benjlevesque's suggestions, after that it's all good for me 👍
The reason for this PR came about when trying to introduce native USDC and bridged USDC to Polygon and Optimism.
Similar fix was done to Arbitrum One here by @alexandre-abrioux.
However, the reason it worked for Arbitrum, and does not work for Polygon and Optimism is because the network name arbitrum-one has a dash in it, the full currency identifier is USDC-arbitrum-one.
Because of the two dashes, the currency parsing from CurrencyManager failed with CurrencyManager.fromSymbol and then was correctly detected by CurrencyManager.fromId.
But with chain names without dashes, the CurrencyManager.fromSymbol worked but it fetches the wrong USDC token, it doesn't prioritize the id, that's why I had to prioritize the CurrencyManager.fromId with this PR.
I think it's the more correct way of doing things because the id is exact match, if the exact match fails, then we can attempt to detect based on the symbol with or without a network etc.
Further Context on the USDC tokens in Asana Task