fix: GetSupplyWithOffset returns int #308
Conversation
| @@ -32,7 +32,7 @@ type Keeper interface { | |||
| IterateTotalSupply(ctx sdk.Context, cb func(sdk.Coin) bool) | |||
| GetSupplyOffset(ctx sdk.Context, denom string) sdk.Int | |||
| AddSupplyOffset(ctx sdk.Context, denom string, offsetAmount sdk.Int) | |||
| GetSupplyWithOffset(ctx sdk.Context, denom string) sdk.Coin | |||
There was a problem hiding this comment.
Why do we need this change? I think we might want to maintain compatibility with APIs in the upstream
There was a problem hiding this comment.
The problem is we need to be able to return a negative number and we can only do this with an int, otherwise we cannot compare the negative diff if we use Coin because Coin can be at a minimum 0
There was a problem hiding this comment.
Why would supply with offset ever be negative? In my opinion, supply should not be negative by definition
The only reason we have the offset is because we over minted by 225m OSMO at genesis. So we are offsetting these 225m with the offset. Therefore, supply with offset should always be >= 0. If it's negative, there is probably a problem / misconfigiration somewhere
There was a problem hiding this comment.
Ahh I see - so the claim is that it can be negative in theory so we would like to allow for that?
What is the use case for capturing this? Why would we need to capture by how much it is negative? In Osmosis x/mint with mainnet parameters supply with offset should never be negative
There was a problem hiding this comment.
I have no idea, this PR introduced tests and logic for negative supply due to offset, and when brought into osmosis causes test failures.
There was a problem hiding this comment.
So should I just refactor the test to require 0 on an expected negative supply result? I can do this it just seemed...wrong
There was a problem hiding this comment.
If that's the only occurrence where this matters, I think we should instead refactor that test and pre-mint 225 million + 1, then SetInitialSupplyOffsetDuringMigration (-225m) and, finally test that the supply with offset == 1 is true.
The reason is that breaking the API compatibility between our fork and the upstream SDK is more difficult to maintain and has higher overhead than adjusting that test.
Sorry I didn't realize this earlier. Please let me know what you think.
There was a problem hiding this comment.
This makes sense! I will close this PR and modify the test osmosis side. Thank you!
There was a problem hiding this comment.
Actually, I just checked that supply with offsets have not been upsteamed.
They only exist on our fork so the compatibility limitation does not exist.
There was a problem hiding this comment.
@czarcas7ic given the above, I think it is more flexible in terms of how we approach this. However, I still do not understand when supply with offset could be negative semantically. From my understanding, if that ever happens, I think we should panic and halt the chain.
Therefore, I'm not sure if allowing for it to be negative via APIs is what we want.
p0mvn
dismissed
their stale review
I would like to understand why it can be negative first
Closes: #XXX
What is the purpose of the change
Previously we made a PR that returned a zero coin value if the resulting supply was negative. This prevents us from checking what this offset actually is in tests such as this
https://github.com/osmosis-labs/osmosis/blob/main/x/mint/keeper/keeper_test.go#L354
Brief Changelog
Testing and Verifying
This change is already covered by existing tests
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? no