Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: adding CoW helper MVP #121
feat: adding CoW helper MVP #121
Changes from 32 commits
699d9d7
271e00a
48888e3
28565f7
1a44f5e
8bf22dd
3683943
9422865
be1701d
24553ab
441d117
5630797
627d878
cb2fb52
5207c18
6f76f85
de99df4
bd64dca
ac17f69
6a14084
96f0302
e8c4d33
22480c5
3e5bfea
68e8368
5afd282
9390e64
b1ba43a
9ec1c34
05d6271
9923fc0
8d72ff8
699996e
1bec1fd
1ffda8b
063d3f6
471b0e6
e0de518
d0bba5c
d8c49e6
abe6f00
b2056be
ffb1bc0
7719c39
7db4e8e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this is not in any interface, IIRC we intended to have every public method available in an interface as part of our internal style guide (although it's disabled in the natspec smells config, since we didn't want to deal with some methods on BMath and BNum)
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.
hmmm we could make it internal tbh, this contract is supposed to be called only by the interface related methods, as it's al off-chain helper for solvers
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.
from what you laid out here: #156 (comment) I suppose we should skip the explicit return
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.
@wei3erHase 👀
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.
i think is worth calling it "non-obvious return", specially since
postInteractions
will be empty and we cannot not declare it (compiler warnings)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.
feels weird to have a constant in between functions, are we sure it's best to disable solhint's
ordering
for tests?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.
I'd do a ranged assert with a computed amount, similar to how you did with
order.{buy,sell}Amount
belowThere 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.
I believe a unit test is enough for this
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.
this one was chosen to maintain the shape in case the feature gets added later, the unit test of the helper reverting is there, and the integration perspective we keep it here, in the future, another helper will not revert on weighted pools, and this test can be reimplemented not to revert
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.
... if this is meant to stay then wdyt about removing the commented code below?