osmosis icon indicating copy to clipboard operation
osmosis copied to clipboard

x/mint: fix rounding issues

Open p0mvn opened this issue 3 years ago • 2 comments

Background

This is a follow-up to https://github.com/osmosis-labs/osmosis/issues/1839

https://github.com/osmosis-labs/osmosis/pull/1874 helped to uncover rounding issues with minting. Please follow the PR description to understand the context.

There are currently 2 discovered rounding issues:

  1. Truncating epoch provisions
  • Line that causes this: https://github.com/osmosis-labs/osmosis/blob/ff383fc1f640bd1c2f902ce147659b23b751285c/x/mint/types/minter.go#L53

Changing this from Dec to Into might not be as simple. Can potentially lead to a larger refactor and a db migration.

  1. Truncating while calculating developer rewards
  • This one causes a much larger delta Line with the problem: https://github.com/osmosis-labs/osmosis/blob/ff383fc1f640bd1c2f902ce147659b23b751285c/x/mint/keeper/keeper.go#L205 where GetProportions truncates the result before returning.

Also, not sure about the impact yet but the test shows that this is the source.

Before addressing this issue, we should first complete DistributeMintedCoin refactor in: https://github.com/osmosis-labs/osmosis/issues/1916

Acceptance Criteria

  • The 2 sources of rounding issues are mitigated
  • Unit tests added to ensure supply correctness
  • The test introduced in #1874 is either refactored or removed
  • Calculate the exact value that we are off by the next upgrade
  • Allocate the missed value to the community pool during the next upgrade: https://github.com/osmosis-labs/osmosis/tree/main/app/upgrades/v11
  • remove references to this issue in the codebase. It is referenced multiple times.

p0mvn avatar Jun 30 '22 18:06 p0mvn

Blocked by #1916

p0mvn avatar Jun 30 '22 18:06 p0mvn

Fixing this isn't as simple due to the dependencies on the cosmos-sdk APIs.

The core problem is that some sdk modules such as x/bank and x/distribution operate on sdk.Coins as opposed to sdk.DecCoins. For example:

I have asked in the sdk about the reasons and plans for changing this: https://github.com/cosmos/cosmos-sdk/issues/7113#issuecomment-1205688807

In the meantime, I think the best workaround would be to have a decimal store entry in the mint module that tracks the rounding delta between epochs. As soon as the delta reaches 1 sdk.Int, we can send it to the community pool and update the rounding delta.

p0mvn avatar Aug 04 '22 19:08 p0mvn