-
Notifications
You must be signed in to change notification settings - Fork 17
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
Refactor stop-loss create just 1 discrete order #89
Refactor stop-loss create just 1 discrete order #89
Conversation
652e0a2
to
0c673e0
Compare
Co-authored-by: José Ribeiro <[email protected]>
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.
The changes look good, but why was it previously implemented in bucket?
My only guess is that maybe long-lived orders aren't accepted by the API. In case, has this changed?
maxTimeSinceLastOracleUpdate: 15 minutes | ||
}); | ||
|
||
vm.expectRevert( |
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.
It would be better if there was also a test that shows an order is valid if warping to validTo
, all other tests use type(uint256).max
for the valid to so the positive boundary condition isn't tested on a different number.
Because I was foolish when I implemented it 🙈 This was the last conditional order type that I added "last minute". Never do things "last minute" it seems 🤦♂️ |
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.
Concur with the review comments from @fedgiac
@mfw78 once this is merged, let us know if we should deploy this contract on all chains or you will do it. |
Co-authored-by: Federico Giacon <[email protected]>
Description
Changes
validityBucketTime
parameter of stop loss data with constantvalidTo
.How to test
0xe6CDbC068654C506424F7747357F51d0e7caB00e