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

Allow interactions to be internal #195

Merged
merged 2 commits into from
May 24, 2022
Merged

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented May 6, 2022

Now that we have external solvers, we will start requiring them to provide execution/calldata for AMM interactions that they internalize.

This PR changes the exec_plan that is returned from HTTP solvers to instead use an enum to indicate whether or not it is "internal" or to be included in the execution plan. Currently, all we do with "internal" interactions is to skip them when adding them to the solution and logging a message indicating that it was internalized.

Once we start recording settlement competitions in the database, we need to make sure that these internalized AMM interactions get stored somewhere so that they can be verified at a later date. For this, some additional refactoring is necessary in order to make encoding interactions into settlements easier.

One additional point of interest is that we currently don't verify that internalized interactions are indeed over the list of "allowed buffer trading" tokens. I plan on adding this as an follow up PR to not over-complicate this one.

Test Plan

Added a unit test that verifies that internal interactions don't get encoded.

@nlordell nlordell requested a review from a team as a code owner May 6, 2022 14:28
@codecov-commenter
Copy link

codecov-commenter commented May 6, 2022

Codecov Report

Merging #195 (9c66570) into add-amounts-to-interaction (f4ac00d) will increase coverage by 0.37%.
The diff coverage is 90.12%.

❗ Current head 9c66570 differs from pull request most recent head 72145cb. Consider uploading reports for the commit 72145cb to get more accurate results

@@                      Coverage Diff                       @@
##           add-amounts-to-interaction     #195      +/-   ##
==============================================================
+ Coverage                       64.44%   64.81%   +0.37%     
==============================================================
  Files                             189      190       +1     
  Lines                           38988    39147     +159     
==============================================================
+ Hits                            25125    25375     +250     
+ Misses                          13863    13772      -91     

/// The interacetion should **not** be included in the settlement as
/// internal buffers will be used instead.
#[serde(with = "execution_plan_internal")]
Internal,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just one thought:
maybe, we might want to simulate the transactions with the "later to be internalized AMM interactions", in order to see that the internalization was legit at some point.
If we want to do that, I think its better to also ask the solvers for the executionplancoordinates even for internalized AMMs.
But given that the game of the rules are not yet finalized, I dont' know how important a possible simulation really is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, good point.

crates/shared/src/http_solver/model.rs Outdated Show resolved Hide resolved
@nlordell nlordell force-pushed the add-amounts-to-interaction branch from a4f98eb to f4ac00d Compare May 24, 2022 14:38
@nlordell nlordell force-pushed the add-interaction-private-flag branch from 5554a2d to 72145cb Compare May 24, 2022 15:11
Base automatically changed from add-amounts-to-interaction to main May 24, 2022 15:15
Nicholas Rodrigues Lordello added 2 commits May 24, 2022 17:18
@nlordell nlordell force-pushed the add-interaction-private-flag branch from 72145cb to b1ae8eb Compare May 24, 2022 15:18
@nlordell nlordell enabled auto-merge (squash) May 24, 2022 15:19
@nlordell nlordell merged commit f35332c into main May 24, 2022
@nlordell nlordell deleted the add-interaction-private-flag branch May 24, 2022 15:22
@github-actions github-actions bot locked and limited conversation to collaborators May 24, 2022
.flat_map(|u| &u.execution)
.all(|u| u.exec_plan.is_some())
.flat_map(|u| u.execution.iter().map(|e| &e.exec_plan));
let interaction_executions = self.interaction_data.iter().map(|i| &i.exec_plan);
Copy link
Contributor

Choose a reason for hiding this comment

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

This turns out to be a breaking change. The otex solver was not providing an execution plan for its single interaction (it's marked optional on the struct) and therefore stopped being considered in shadow/barn mode and will basically break in prod if we release today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing. Fixed in #237

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants