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

Verify order executed amount in settlement encoder #164

Merged
merged 1 commit into from
Apr 26, 2022
Merged

Conversation

vkgnosis
Copy link
Contributor

Fixes #163

@josojo wrote

I think we should make sure that the for a partially fillable order the "executed amount <= sell amount" and for a killorfill order "executed amount== sell_amount".

But I think for buy orders the executed amount refers to the buy amount.

Test Plan

Adapted the existing tests which showed that the verification worked

Comment on lines +562 to +563
(true, OrderKind::Sell) => executed_amount <= order.creation.sell_amount,
(true, OrderKind::Buy) => executed_amount <= order.creation.buy_amount,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think those conditions can be even stricter since partially fillable orders could already be partially filled.
executed_amount <= (order.creation.sell_amount - order.metadata.executed_sell_amount)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I was thinking this would be detected during simulation because the contract enforces it too but it probably makes sense to catch it as early as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a bit annoying because the other number is a BigInt so would have to convert and then decide what to do if that value doesn't make sense like saturating sub so I left it out for now. Will think again how I want to do this.

@vkgnosis vkgnosis requested a review from josojo April 21, 2022 13:01
@vkgnosis
Copy link
Contributor Author

@josojo could you review this since you made the original issue? Also the part I quote in the description.

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.

Yes, great. Pr looks good. Thanks for modifying it for buy orders. Makes sense to me for sure.

@codecov-commenter
Copy link

Codecov Report

Merging #164 (6e0a624) into main (1738304) will increase coverage by 0.01%.
The diff coverage is 91.66%.

❗ Current head 6e0a624 differs from pull request most recent head b4fe284. Consider uploading reports for the commit b4fe284 to get more accurate results

@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
+ Coverage   64.82%   64.84%   +0.01%     
==========================================
  Files         185      185              
  Lines       38393    38386       -7     
==========================================
+ Hits        24888    24890       +2     
+ Misses      13505    13496       -9     

@vkgnosis vkgnosis enabled auto-merge (squash) April 26, 2022 14:06
@vkgnosis vkgnosis merged commit 1416209 into main Apr 26, 2022
@vkgnosis vkgnosis deleted the verify-ex branch April 26, 2022 14:07
nargeslein pushed a commit to nargeslein/services that referenced this pull request Apr 29, 2022
nargeslein pushed a commit to nargeslein/services that referenced this pull request Apr 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Executed amounts - sanity checks
5 participants