-
Notifications
You must be signed in to change notification settings - Fork 56
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
fix: Pool Coin Decimal Truncation During Deposit #446
fix: Pool Coin Decimal Truncation During Deposit #446
Conversation
due to the change in deposit logic, expected value in test also changed
Codecov Report
@@ Coverage Diff @@
## develop #446 +/- ##
===========================================
- Coverage 85.10% 85.09% -0.01%
===========================================
Files 23 23
Lines 2618 2604 -14
===========================================
- Hits 2228 2216 -12
+ Misses 226 225 -1
+ Partials 164 163 -1
Continue to review full report at Codecov.
|
1d0d2e2
to
e7913a1
Compare
* commit '9e955b0b7dba4c4e9a09e6d2415ce4181b62e90b': feat: bump cosmos sdk to v0.44.1 feat: add markdown link checker github workflow
e7913a1
to
fb4cb94
Compare
* develop: feat: bump cosmos-sdk to v0.44.2
sdk.NewCoin(depositCoins[0].Denom, lastReserveCoinA.Amount.ToDec().Mul(mintRate).TruncateInt()), | ||
sdk.NewCoin(depositCoins[1].Denom, lastReserveCoinB.Amount.ToDec().Mul(mintRate).TruncateInt()), |
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.
sdk.NewCoin(depositCoins[0].Denom, lastReserveCoinA.Amount.ToDec().Mul(mintRate).TruncateInt()), | |
sdk.NewCoin(depositCoins[1].Denom, lastReserveCoinB.Amount.ToDec().Mul(mintRate).TruncateInt()), | |
sdk.NewCoin(depositCoins[0].Denom, lastReserveCoinA.Amount.ToDec().MulTruncate(mintRate).TruncateInt()), | |
sdk.NewCoin(depositCoins[1].Denom, lastReserveCoinB.Amount.ToDec().MulTruncate(mintRate).TruncateInt()), |
What do you think about using MulTruncate
instead of Mul
here? @hallazzang @typark391 @kogisin
I think both will have the same result but that is more intuitive, or is there a case that is not?
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.
Using MulTruncate
here can cause(very rarely) a depositor to take advantage against the pool.
Since the result of MulTruncate
is always less or equal than the result of Mul
, I suggest to use Mul
here.
How do others think?
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.
That's right. Let's keep it Mul
first, reproduce the corresponding edge case, analyze it, and specify it on spec through other PRs.
Logic |
* feat: add markdown link checker github workflow * feat: bump cosmos sdk to v0.44.1 * feat: bump cosmos-sdk to v0.44.2 * fix: Pool Coin Decimal Truncation During Deposit (#446) * fix: wip poc for reproduce and fix poolcoin truncation * fix: simplify calculation logic and add more tests * fix: use equality check in MintingPoolCoinsInvariant * test: fix expected value due to the change in deposit logic, expected value in test also changed * test: add test for MintingPoolCoinsInvariant * fix: update deposit truncation logic and simulation ordering * docs: update changelog and readme * fix: revert MulTruncate to Mul on Deposit Co-authored-by: Hanjun Kim <[email protected]> * Fix: add overflow checking and test codes for cover edge cases (#458) * test: add testcase for cover small withdrawal case * test: add test case for CreatePool * fix: refactor and optimize depleted pool validation * feat: add overflow checking logic * chore: add testcase and remove comments * test: add test code for big deposit * fix: apply PR suggestions * fix: add overflow checking logic and test cases Co-authored-by: Hanjun Kim <[email protected]> Co-authored-by: typark391 <[email protected]> Co-authored-by: kogisin <[email protected]> Co-authored-by: Hanjun Kim <[email protected]> Co-authored-by: typark391 <[email protected]>
* feat: add markdown link checker github workflow 9d729d0 * Merge pull request tendermint#453 from b-harvest/kogisin/451-bump-sdk-v0.44.1 feat: bump Cosmos SDK from v0.44.0 to v0.44.1 (cherry picked from commit 9e955b0) * Merge pull request tendermint#455 from b-harvest/454-bump-sdk-v0.44.2 feat: bump cosmos-sdk to v0.44.2 (cherry picked from commit e118e21) * fix: Pool Coin Decimal Truncation During Deposit (tendermint#446) * fix: wip poc for reproduce and fix poolcoin truncation * fix: simplify calculation logic and add more tests * fix: use equality check in MintingPoolCoinsInvariant * test: fix expected value due to the change in deposit logic, expected value in test also changed * test: add test for MintingPoolCoinsInvariant * fix: update deposit truncation logic and simulation ordering * docs: update changelog and readme * fix: revert MulTruncate to Mul on Deposit Co-authored-by: Hanjun Kim <[email protected]> (cherry picked from commit 616985f) * Fix: add overflow checking and test codes for cover edge cases (tendermint#458) * test: add testcase for cover small withdrawal case * test: add test case for CreatePool * fix: refactor and optimize depleted pool validation * feat: add overflow checking logic * chore: add testcase and remove comments * test: add test code for big deposit * fix: apply PR suggestions * fix: add overflow checking logic and test cases
Description
closes: #445
For now, it is a draft version that reproduces the above issue and solves it at the PoC level, requiring more testing and edge case simulation.
Tasks
ExecuteDeposit
functionMintingPoolCoinsInvariant
andDepositInvariant
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes