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.

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

Merged
merged 18 commits into from
Apr 29, 2024
Merged

Domain Settlement Encoding #2661

merged 18 commits into from
Apr 29, 2024

Conversation

fleupold
Copy link
Contributor

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

  • Introduce encoding module which exposes a function to take a solution and returns a settlement tx object
  • Encode clearing prices, trades & interactions the way the settlement contract expects (guided by the boundary implementation)
  • Implement revertable logic on Solutions (this also fixes feat: Always classify settlements containing ERC1271 orders as unsafe #2640)
  • Make allowance computation internalisation aware (currently allowances for internalised interactions are added to the execution plan even if we are building an internalised one)
  • Unify swap implementation of the liquidity types to always return Result (one returned Option)

How to test

  • Change default 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

@fleupold fleupold requested a review from a team as a code owner April 25, 2024 08:50
crates/driver/src/domain/competition/solution/encoding.rs Outdated Show resolved Hide resolved
crates/driver/src/domain/competition/solution/encoding.rs Outdated Show resolved Hide resolved
crates/driver/src/domain/competition/solution/encoding.rs Outdated Show resolved Hide resolved
crates/driver/src/domain/competition/solution/encoding.rs Outdated Show resolved Hide resolved
crates/driver/src/domain/competition/solution/encoding.rs Outdated Show resolved Hide resolved
crates/driver/src/domain/competition/solution/encoding.rs Outdated Show resolved Hide resolved
crates/driver/src/domain/competition/solution/encoding.rs Outdated Show resolved Hide resolved
crates/driver/src/domain/competition/solution/encoding.rs Outdated Show resolved Hide resolved
crates/driver/src/domain/competition/solution/encoding.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@squadgazzz squadgazzz left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@MartinquaXD MartinquaXD left a 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.

@sunce86
Copy link
Contributor

sunce86 commented Apr 26, 2024

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.

@fleupold fleupold force-pushed the prepare_domain_submission_config branch from 96ce9c5 to 6889cf4 Compare April 27, 2024 11:42
@fleupold fleupold force-pushed the domain_settlement_encoding branch from 8183102 to 232600a Compare April 27, 2024 12:31
@fleupold
Copy link
Contributor Author

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.

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.

@fleupold fleupold mentioned this pull request Apr 27, 2024
4 tasks
Base automatically changed from prepare_domain_submission_config to main April 29, 2024 07:00
@fleupold fleupold enabled auto-merge (squash) April 29, 2024 15:45
@fleupold fleupold merged commit 6bb059b into main Apr 29, 2024
10 checks passed
@fleupold fleupold deleted the domain_settlement_encoding branch April 29, 2024 16:07
@github-actions github-actions bot locked and limited conversation to collaborators Apr 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants