Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
👋 PabloMansanet, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
Coverage Report |
aelmanaa
previously requested changes
Mar 3, 2026
PabloMansanet
changed the title
PabloMansanet
changed the title
aelmanaa
requested changes
Mar 4, 2026
andrevmatos
reviewed
Mar 4, 2026
andrevmatos
requested changes
Mar 4, 2026
Collaborator
andrevmatos
left a comment
andrevmatos
left a comment
There was a problem hiding this comment.
Proposed some changes, to allow the existing implementation of getLaneFeatures to use only existing methods (instead of hitting the contracts directly).
With those changes, I think we can move this implementation to the concrete generic implementation inside Chain.getLaneFeatures, and will serve everyone (i.e. once they start supporting v2, it is).
Comment on lines
+661
to
+664
| const tokenPool = (await onRampContract.getPoolBySourceToken( | ||
| opts.destChainSelector, | ||
| opts.token, | ||
| )) as string |
Collaborator
There was a problem hiding this comment.
Can we use the existing method instead of hitting the contract directly? This may make it more generic in the future
Suggested change
| const tokenPool = (await onRampContract.getPoolBySourceToken( | |
| opts.destChainSelector, | |
| opts.token, | |
| )) as string | |
| const { tokenPool } = await this.getRegistryTokenConfig( | |
| await this.getTokenAdminRegistryFor(onRamp), | |
| opts.token, | |
| ) |
Contributor
Author
There was a problem hiding this comment.
This does a lot more thoug, right? The registry path does typeAndVersion (line 997) + getStaticConfig (line 1017) + getTokenConfig (line 1434) = 3 calls, vs the current single getPoolBySourceToken call.
Contributor
Author
There was a problem hiding this comment.
As per our chat: let's approach in a followup
Comment on lines
+675
to
+680
| const poolContract = new Contract( | ||
| tokenPool, | ||
| interfaces.TokenPool_v2_0, | ||
| this.provider, | ||
| ) as unknown as TypedContract<typeof TokenPool_2_0_ABI> | ||
| const minBlockConfirmations = Number(await poolContract.getMinBlockConfirmations()) |
Collaborator
There was a problem hiding this comment.
Just for consistency, can we add minBlockConfirmations? as part of the EVM-specific return type of getTokenPoolConfig method, return it in there if pool is >=v2, and then use that method here?
Contributor
Author
There was a problem hiding this comment.
Hmm I'm not sure about this one, I think it confuses things a bit for the sake of consistency. minBlockConfirmations is its own method in the ABI and it's not marked as "config" anywhere. Just like it's lane features and not lane config, I don't think this value should come from getTokenPoolConfig
Contributor
Author
There was a problem hiding this comment.
As per our chat: let's approach in a followup
Collaborator
There was a problem hiding this comment.
Ok, please, just don't forget it 😅
andrevmatos
approved these changes
Mar 4, 2026
Comment on lines
+675
to
+680
| const poolContract = new Contract( | ||
| tokenPool, | ||
| interfaces.TokenPool_v2_0, | ||
| this.provider, | ||
| ) as unknown as TypedContract<typeof TokenPool_2_0_ABI> | ||
| const minBlockConfirmations = Number(await poolContract.getMinBlockConfirmations()) |
Collaborator
There was a problem hiding this comment.
Ok, please, just don't forget it 😅
aelmanaa
approved these changes
Mar 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fully fork tested, with a caveat:
There was a recent rename of the token pool method
getMinBlockConfirmation()togetMinBlockConfirmations()(note the plural). To keep with the patterns in this codebase, we only generate our token pool ABI based on the latest upstream code, so we only check for the existence of the plural.Current staging deployments are mostly on the older naming, with only the Lombard LBTC pool (as far as I know) using the plural form. This means our fork tests can only check the Lombard case (FTF disabled). As soon as more pools migrate, we'll be able to add fork tests that verify FTF support in other pools.