-
Notifications
You must be signed in to change notification settings - Fork 13
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
Minimal Vault, Router, Transient Accounting #81
Conversation
Would it help to start merging this to keep the house clean? We can always go back and / or iterate over it. @ylv-io @danielmkm @EndymionJkb WDYT? |
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.
Just two more open questions so as not to lose sight of the original problem in V2, and triple check.
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 done one more pass while wrapping my head around trying to figure out how to best document this.
I've picked up mostly nits and silly questions, and just a few important comments :)
And I have to say that I come back to this and I'm yet again impressed by the design; great job!
Co-authored-by: Juan Ignacio Ubeira <juani@balancerlabs.dev>
Co-authored-by: Juan Ignacio Ubeira <juani@balancerlabs.dev>
…f multiple lines
Co-authored-by: Juan Ignacio Ubeira <juani@balancerlabs.dev>
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.
Forgot to submit...
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.
🚀 let's gooo!
I think this is good to go. I'd suggest merging, @EndymionJkb @danielmkm please feel free to take a look after the PR is closed if you want to, but I think it's a good time to move forward.
Handlers, were a temp name. I like Routers, what do you think @jubeira ? |
But min amounts out are not invariants and do not affect Vault security. It is true that they may result to users losing money on horrible trades. If we want to keep Vault minimal it seems okay to put this in the Router. |
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.
Added some comments, but agree that we should get this merged. It's hard to get a full sense of the current state based on this PR alone anymore.
…ountsIn instead of maxAmountsIn and update the return variable names. Also, remove the check for minBptAmountOut in Router.sol.
…IVault.sol and Vault.sol contracts
…nction in Vault.sol for better readability and maintainability.
Description
This PR introduces a new design comprised of three contracts:
Router<>Vault<>Pool
. This design parallels the previousRelayer<>Vault<>Pool
structure. The key distinction lies in the Vault API, which is more internally driven than user-centric. Another design feature is transitional accounting. This approach enables multiple operations within a transaction, settling token balances only at the conclusion. It also permits arbitrary calls to other contracts amidst vault operations, thereby facilitating use-cases such as 'addLiquidity' followed by 'stake'.The benefits of this model include:
The Vault API offers functions like
settle
,wire
,mint
,retrieve
, andburn
to finalize balances. These functions ensure token transfers occur at a transaction's end, settling any outstanding balances. Concurrently, they enable flash loans; for instance, one canmint
a specific quantity of token A and exchange it. This token A amount would only require repayment at the transaction's conclusion.The Vault's security is of utmost importance. Therefore, the integrity of its logic is non-negotiable. The
transient
modifier serves this purpose. It's a potent code ensuring two end-of-transaction invariants: a) all accounts are settled to zero; if not, aBalanceNotSettled
alert is triggered and all modifications are reverted, and b) it confirms we are the final handler.Type of change
Checklist:
main
, or there's a description of how to mergeIssue Resolution
Closes #39.
Closes #40.
Closes #41.