-
Notifications
You must be signed in to change notification settings - Fork 64
fix: incorrect token information in response for wetheth #1867
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: incorrect token information in response for wetheth #1867
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
}); | ||
|
||
test("should return WETH for Base and Linea", async () => { | ||
const params = { |
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.
This is the original sample request from the Linear ticket:
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.
NIT: I also didn't specify it in the ticket but what happens if you set the zero address as the inputToken
? It seems that this also resolves to the wrong token information. But as above we can tackle this in a separate PR/issue
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.
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 created in issue for this here: https://linear.app/uma/issue/ACX-4504/fix-the-incorrect-token-information-for-all-native-tokens
…on-in-response-for-wetheth
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.
Nice work! While reviewing I noticed two cases that were not covered in the Linear issue. We can tackle them separately
// See: https://www.npmjs.com/package/@across-protocol/constants | ||
// This can cause issues when resolving the token. | ||
// To fix this, we will check if there is a WETH match and prioritize it over the ETH match. | ||
const wethMatch = matches.find(([symbol]) => symbol === "WETH"); |
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.
NIT: We didn't specify it in the ticket but now that I review this, this bug happens for all native tokens, e.g. XPL, GHO. So this fix will not work for them 🤔 We can tackle this is in a separte PR/issue though
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.
}); | ||
|
||
test("should return WETH for Base and Linea", async () => { | ||
const params = { |
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.
NIT: I also didn't specify it in the ticket but what happens if you set the zero address as the inputToken
? It seems that this also resolves to the wrong token information. But as above we can tackle this in a separate PR/issue
Fix WETH/ETH Symbol Resolution
Linear: ACX-4369
📝 Summary
This pull request addresses a bug where the
swap/approval
endpoint was incorrectly resolving the symbol for wrapped native tokens (like WETH) as the native token symbol (e.g., ETH). This was happening on chains like Base and Linea where the wrapped native token address is also present in theTOKEN_SYMBOLS_MAP
under theETH
symbol.🔧 Changes
getTokenByAddress
function inapi/_utils.ts
to correctly resolve the token symbol in cases of ambiguity betweenETH
andWETH
.getTokenByAddress
to ensure the correct symbol is returned for all supported chains.💭 Rationale
The root cause of this issue lies in the
@across-protocol/constants
package, where some wrapped native token addresses are associated with bothETH
andWETH
symbols. ThegetTokenByAddress
function was not deterministically picking the correct symbol, sometimes defaulting toETH
when it should have beenWETH
.This pull request implements a solution that prioritises
WETH
when a token address is associated with both symbols. This is a robust solution because it correctly handles the ambiguity in the constants file and ensures that the correct token symbol is used throughout the application.The alternative of fixing the constants file is not feasible as it is an external dependency. This solution provides a local fix that is both effective and maintainable.