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

Require input and output amounts for raw interactions #194

Merged
merged 5 commits into from
May 24, 2022

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented May 6, 2022

This PR augments interaction data to also include input and output amounts. This will be required by the driver in order to verify token conservation across tokens and orders.

Test Plan

Added unit test to verify de-serialization. For now, there is no new logic changes.

Release notes

This PR introduces changes to the HTTP solver interface. Note that the API was kept backwards compatible to allow solver implementations time to adapt to the new API. These changes will be made required after a certain amount of time (since they are required for validating new solver rules around local and global token conservation).

@nlordell nlordell requested a review from a team as a code owner May 6, 2022 11:45
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.

The code itself looks good (nice refactor of TokenAmount).

verify token conservation across tokens and orders...

Could harm be done by solvers lying about the input and output amounts?
I guess we would have to simulate each interaction to know whether those values are correct, right?

Copy link
Contributor

@josojo josojo left a comment

Choose a reason for hiding this comment

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

Looks good!

crates/shared/src/http_solver/model.rs Outdated Show resolved Hide resolved
@nlordell
Copy link
Contributor Author

nlordell commented May 6, 2022

Could harm be done by solvers lying about the input and output amounts?

Yes. What is unclear is whether or not we should verify this upfront (by simulating as you suggested) or if we should slash solvers that misbehave.

@codecov-commenter
Copy link

codecov-commenter commented May 6, 2022

Codecov Report

Merging #194 (f4ac00d) into main (56ae162) will increase coverage by 0.08%.
The diff coverage is 95.45%.

❗ Current head f4ac00d differs from pull request most recent head 997edc0. Consider uploading reports for the commit 997edc0 to get more accurate results

@@            Coverage Diff             @@
##             main     #194      +/-   ##
==========================================
+ Coverage   64.35%   64.44%   +0.08%     
==========================================
  Files         189      189              
  Lines       38896    38988      +92     
==========================================
+ Hits        25032    25125      +93     
+ Misses      13864    13863       -1     

Copy link
Contributor

@vkgnosis vkgnosis left a comment

Choose a reason for hiding this comment

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

I guess this has been discussed already but doesn't this limit what an interaction can do in a potential important manner? What if you have an interaction with one input amount and two output amounts or vice versa? What if a first interaction facilitates a next interaction and either one alone doesn't have the proper in and out amounts?

@nlordell
Copy link
Contributor Author

nlordell commented May 6, 2022

What if you have an interaction with one input amount and two output amounts or vice versa?

Good point. I guess these should be inputs: Vec<TokenAmount> and outputs: Vec<TokenAmount>.

What if a first interaction facilitates a next interaction and either one alone doesn't have the proper in and out amounts?

Then, given ☝️, the first interaction be modelled as {... input: [], output: [] ...} and the second one containing a list of all input and output token amounts.

@nlordell nlordell requested a review from MartinquaXD May 16, 2022 14:59
@nlordell
Copy link
Contributor Author

nlordell commented May 24, 2022

I changed this PR to make the changes backwards compatible. This way we can start implementing the external solver checks (local and global token conservation) without breaking existing solvers while also allowing our existing solvers to change to use this new API.

@MartinquaXD would appreciate a re-review.

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.

Does still look good to me. 👍

crates/shared/src/http_solver/model.rs Outdated Show resolved Hide resolved
@nlordell nlordell enabled auto-merge (squash) May 24, 2022 15:14
@nlordell nlordell merged commit 169ffe2 into main May 24, 2022
@nlordell nlordell deleted the add-amounts-to-interaction branch May 24, 2022 15:15
@github-actions github-actions bot locked and limited conversation to collaborators May 24, 2022
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.

5 participants