Skip to content
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.

Sign up for GitHub

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

Jump to bottom

getting on-chain data via multicall3 with soft fails and batching #526

Merged
merged 13 commits into from
Sep 1, 2023
Merged

getting on-chain data via multicall3 with soft fails and batching #526

merged 13 commits into from
Sep 1, 2023

Conversation

gmbronco
Copy link
Contributor

No description provided.

brunoguerios
brunoguerios reviewed Aug 25, 2023
View reviewed changes
Copy link
Member

@brunoguerios brunoguerios left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some questions/suggestions, but overall looks good to me 👍

balancer-js/src/modules/data/pool/subgraph.ts Outdated Show resolved Hide resolved
balancer-js/src/modules/pools/queries/queries.integration.spec.ts Show resolved Hide resolved
balancer-js/src/modules/sor/pool-data/onChainData3.ts Outdated Show resolved Hide resolved
balancer-js/examples/data/onchain-multicall3.ts Show resolved Hide resolved
balancer-js/examples/data/onchain-multicall3.ts Outdated Show resolved Hide resolved
balancer-js/src/modules/sor/pool-data/onChainData3.ts Show resolved Hide resolved
@johngrantuk
Copy link
Member

The comments I left for specific pool types/versions - we're making the calls even though we know they'll fail right? Wonder if we should just be more explicit there? Also in general should we be highlighting/logging failed calls which might show wider issues?

@johngrantuk
Copy link
Member

@gmbronco - not sure why the tests are failing? They don't seem to on develop.

@gmbronco gmbronco force-pushed the onchaindata-multicall3 branch 2 times, most recently from eaf3cc1 to 9c58703 Compare August 31, 2023 10:04
johngrantuk
johngrantuk approved these changes Sep 1, 2023
View reviewed changes
Copy link
Member

@johngrantuk johngrantuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to go I think. Thanks! Can let the front-end know once its released and would be worth keeping an eye for any bugs.

@johngrantuk johngrantuk mentioned this pull request Sep 1, 2023
Draft
@gmbronco gmbronco merged commit 88ca870 into develop Sep 1, 2023
3 checks passed
@johngrantuk johngrantuk mentioned this pull request Sep 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brunoguerios brunoguerios brunoguerios left review comments

@johngrantuk johngrantuk johngrantuk approved these changes

@lgahdl lgahdl Awaiting requested review from lgahdl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gmbronco @johngrantuk @brunoguerios