-
Notifications
You must be signed in to change notification settings - Fork 8
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
Update balancerPoolDataQueries to latest contract code #73
Conversation
@danielmkm I know the deployment addresses are generated from the monorepo deployments pipeline. But can we either override that with a manual deploy or add in |
const rpcUrl = | ||
process.env['ETHEREUM_RPC_URL'] || 'https://eth.llamarpc.com'; |
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've added a fallback RPC as otherwise viem would need a chain passed along to figure out which public RPC to use. Could also change by passing the chain ID to the OnChainPoolDataEnricher.
test/sor.test.ts
Outdated
@@ -55,7 +58,7 @@ describe('SmartOrderRouter', () => { | |||
); | |||
|
|||
const swapOptions: SwapOptions = { | |||
block: 16900000n, | |||
block: 17473810n, |
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.
@danielmkm had to change the block number here so the new contract can be used. Now the two boosted swap tests don't return correct values. I'd assume that is an error in the SOR logic? If i change the block number on the main branch, the same thing happens.
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 believe this might be because we're not handling protocol fees.
With the old block number the path is using a CS V1 pool which is in recovery mode at that block. With the updated block number it uses a CS V3 pool which is not in recovery mode. I'll check out if I can add whats required.
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.
Was actually an issue with the amp. I proposed a fix in #81.
I'll talk to juani, see if they've got a strategy here of if they'll just let me overwrite the addresses. |
@danielmkm updated the code here to the latest main code. Tests run now. What is the status of the create2 deployment of the query contract? Also, how does that work for the Fantom deployments? |
The following pool data can now be queried in addition:
I also renamed the ABI and references to balancerPoolDataQuery.
Question: The helper contract returns an "ignore" array with indexes for each pool that threw an error on a query. The data for that query is then usually set to 0 (or false). What I did is add a field called
queryFailed
to the return type which indicates whether this pool had a failed query or not so the calling service would need to handle it. I wasn't sure if it should be returned with this indication or if it should not be returned at all. What do you think?