-
Notifications
You must be signed in to change notification settings - Fork 380
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
Relayer: query swap/join/exit (V2) #2521
Conversation
I would lean towards this implementation over the one in #2516 if the bytecode limitations can be overcome. imo, it's easier to inform someone to call It could also be useful to change the name |
I think this will provide a huge quality of life improvement generally, but the There is no way around this, but I would expect the limitation to be clarified somehow, whereas currently this would only be found after some serious debugging by an integrator |
* @notice Allows users to simulate the core functions on the Balancer Vault (swaps/joins/exits), using queries instead | ||
* of the actual operations. | ||
*/ | ||
abstract contract VaultQueryActions is VaultActions { |
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 like that you maximize code reuse by implementing VaultActions
. 👍
cdb90fa
to
64183ca
Compare
# Conflicts: # pkg/standalone-utils/contracts/relayer/BalancerRelayer.sol # pkg/standalone-utils/contracts/relayer/BaseRelayerLibrary.sol # pvt/benchmarks/relayer.ts
--When calling multicall with vault authorisation signature, the relayer fails with 430 (low level failure). Not sure if that's intended? Should I add a test case?--- My bad, I was using a relayer library address at initialization. |
# Conflicts: # pkg/standalone-utils/test/VaultActions.test.ts
I've been using a locally deployed relayer from this branch and I identified an issue when querying for chained exits.
for the query call, if I set the output references as:
and peek their values, it actually returns values for token indexes |
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.
Great job! I only have a few nits as comments; otherwise LGTM.
Can we please add a test for the fix in a958b17?
@brunoguerios I understand you've tested the fix already. Do you have any other findings to share with us, or do you think this is good to go?
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.
LGTM, thanks @EndymionJkb !
The error in CI is unrelated; probably an issue with Foundry's nightly release.
Let's proceed with the PR, and get one final thumbs up from @brunoguerios before deploying.
My tests are all passing with a relayer deployed from the latest commit, so on my end it's ok 👍 |
Description
This is a full version of the Relayer: except it also exposes a queryMulticall that does queries instead of actual operations, using a second "stripped down" relayer deployment that only implements query Vault actions. It's an alternative to PR #2516, which is a "drop-in" relayer that would be called instead of the standard (full featured) one.
Tests check that calls to the queryMulticall give the same results as BalancerQueries, and do not require approvals.
Type of change
Checklist:
master
, or there's a description of how to mergeIssue Resolution
Closes #2209