-
Notifications
You must be signed in to change notification settings - Fork 30
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
[TRD-783] Implement CalcAmountIn for kyber limit order #657
[TRD-783] Implement CalcAmountIn for kyber limit order #657
Conversation
d3452a4
to
7af5090
Compare
Test coverage changes:
|
fcc721d
to
a45bf52
Compare
68572b4
to
adf23e3
Compare
filledTakingAmountWei := totalAmountInAfterFee | ||
filledMakingAmountWei, _ := amountOutWei.Int(nil) | ||
filledMakingAmountWei := new(big.Int).Div( | ||
new(big.Int).Mul(filledTakingAmountWei, remainingMakingAmountWei), |
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.
should be order's original makingAmount/takingAmount instead of the remaining amounts, there might be no different at all but it's better to follow SC logic https://github.com/KyberNetwork/ks-limit-order-sc/blob/develop/src/offchain/helpers/AmountCalculator.sol#L34
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.
My bad. I've updated
assert.Equal(t, nil, err) | ||
got, err := testutil.MustConcurrentSafe[*pool.CalcAmountInResult](t, func() (any, error) { | ||
limit := swaplimit.NewInventory("", p.CalculateLimit()) | ||
return p.CalcAmountIn( |
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.
maybe you can call CalcAmountOut
on the result here, and check if the result is equal to tt.args.tokenAmountOut
(or different within 1-2 wei)
or even better, write a random test to asset that CalcAmountOut(CalcAmountIn(random_input))
is equal or close to random_input
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've added more tests for calling both CalcAmountOut
and CalcAmountIn
with same input.
Reduce the length of tests 2 Reduce the length of tests 3
7884316
to
2b1b81d
Compare
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.
sorry I don't have time to review full CalcAmountIn
logic right now, but given that the reversed test cases (out->in->out) have passed I think we're good to go
Why did we need it?
Related Issue
Release Note
How Has This Been Tested?
Screenshots (if appropriate):