-
Notifications
You must be signed in to change notification settings - Fork 245
fix: always sends two async calls when the tokenIn or tokenOut is either ETH or WETH #1255
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
base: main
Are you sure you want to change the base?
fix: always sends two async calls when the tokenIn or tokenOut is either ETH or WETH #1255
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Graphite Automations"Request reviewers once CI passes on routing-api repo" took an action on this PR • (11/06/25)4 reviewers were added and 1 assignee was added to this PR based on Siyu Jiang (See-You John)'s automation. |
|
what dos this mean exactly? all tokens have permit2 right? |
| }, | ||
| queryStringParameters: { | ||
| tokenInAddress: getSymbolOrAddress(partitionKey.currencyIn, partitionKey.chainId), | ||
| tokenInAddress: currencyIn.isNative ? currencyIn.symbol : currencyIn.address, |
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 am still having a hard time wrapping my head around this;
is the issue that partitionKey.currencyIn might incorrectly not be classified as addressZero aka the native token, therefore, we won't update correctly?
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 think this does the same thing as before;
when you have a native, you would pass down ADDRESS_ZERO which before would be updated with the symbol. Now, you are passing the symbol just by checking if its native or not, but that sa check you can do based on the currencyIn address (which should be 0)
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 will be reverted
| tokenInAddress: currencyIn.isNative ? currencyIn.symbol : currencyIn.address, | ||
| tokenInChainId: partitionKey.chainId.toString(), | ||
| tokenOutAddress: getSymbolOrAddress(partitionKey.currencyOut, partitionKey.chainId), | ||
| tokenOutAddress: currencyOut.isNative ? currencyOut.symbol : currencyOut.address, |
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 will be reverted
| slippageTolerance: swapOptions?.slippageTolerance?.toFixed(2), | ||
| simulateFromAddress: swapOptions?.simulate?.fromAddress, | ||
| recipient: swapOptions?.recipient, | ||
| deadline: | ||
| parseInt((swapOptions as SwapOptionsUniversalRouter).deadlineOrPreviousBlockhash?.toString() ?? '0') - | ||
| Math.floor(Date.now() / 1000), | ||
| permitSignature: (swapOptions as SwapOptionsUniversalRouter).inputTokenPermit?.signature, | ||
| permitNonce: (swapOptions as SwapOptionsUniversalRouter).inputTokenPermit?.details?.nonce, | ||
| permitExpiration: (swapOptions as SwapOptionsUniversalRouter).inputTokenPermit?.details?.expiration, | ||
| permitAmount: (swapOptions as SwapOptionsUniversalRouter).inputTokenPermit?.details?.amount, | ||
| permitToken: (swapOptions as SwapOptionsUniversalRouter).inputTokenPermit?.details?.token, | ||
| permitSpender: (swapOptions as SwapOptionsUniversalRouter).inputTokenPermit?.spender, | ||
| permitSigDeadline: (swapOptions as SwapOptionsUniversalRouter).inputTokenPermit?.sigDeadline, |
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.
we prob need to keep those
cgkol
left a comment
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.
So with the new changes, you guarantee that you'll pull cache results for both eth/weth everytime you call either, but are we guaranteed that it will update both?
| } | ||
|
|
||
| const result = await this.ddbClient.query(queryParams).promise() | ||
| const additionalQueryParams = DynamoRouteCachingProvider.getAdditionalQueryParams(currencyIn, currencyOut, tradeType, chainId, includesV4Pool, this.routesTableName) |
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.
lets add some comments epxlaining why we do this
…he entry, we'd still send the async call
6780fc5 to
68c55cf
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
cgkol
left a comment
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.
delete code from other places
| alphaRouterConfig, | ||
| swapOptions | ||
| ) | ||
| } else if (currencyIn.isNative || currencyOut.isNative) { |
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 will probably work

Currently, we see two problems with the async lambda call (async caching update):
Desired behavior is:
In order to achieve this, we need to make sure, we always try to query the dynamo with both ETH/* and WETH/* cache entries.