-
Notifications
You must be signed in to change notification settings - Fork 87
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
Domain Settlement Encoding #2661
Conversation
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
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.
Code looks correct. Just a few nits.
Adding a few unit tests where we compare the encoded settlement boundary vs domain would be very nice to prove the new code is indeed a refactor. Example solutions can be taken from prod logs. |
96ce9c5
to
6889cf4
Compare
8183102
to
232600a
Compare
I think the testing of this feature can be seen by the e2e & driver integration tests passing (any handful of solution is also not going to miss subtle differences). What might be a good idea though is to "shadow encode" every boundary settlement with the domain encoder in prod for a while and log if there is any mismatch. |
Co-authored-by: Martin Beckmann <[email protected]>
Description
Main part of #2215. This PR ports the settlement encoding logic which is currently contained in the legacy solver crate's
SettlemenEncoder
to our domain.It is importantly still missing application of slippage tolerance (which is contained in
SlippageContext
, porting which would have made this PR even larger.Changes
revertable
logic onSolutions
(this also fixes feat: Always classify settlements containing ERC1271 orders as unsafe #2640)swap
implementation of the liquidity types to always returnResult
(one returnedOption
)How to test
encoding::Strategy
from Boundary to Domain and run tests.Important
We still need to implement the slippage computation to make this strategy fully working
Fixes #2215 & #2640