Skip to content
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.

Sign up for GitHub

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: GetSupplyWithOffset returns int #308

Closed
wants to merge 4 commits into from
Closed

fix: GetSupplyWithOffset returns int #308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0452466
supply offset to int
czarcas7ic Aug 10, 2022
42ed004
test not handling negative case
czarcas7ic Aug 10, 2022
5e7ceb1
remove previous test
czarcas7ic Aug 10, 2022
4eb7666
uncomment negative check
czarcas7ic Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion x/bank/keeper/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,11 @@ func (q Querier) SupplyOf(c context.Context, req *types.QuerySupplyOfRequest) (*

ctx := sdk.UnwrapSDKContext(c)
supply := q.GetSupplyWithOffset(ctx, req.Denom)
if supply.IsNegative() {
return nil, status.Errorf(codes.OutOfRange, "resulting supply is negative: %v", supply)
}

return &types.QuerySupplyOfResponse{Amount: sdk.NewCoin(req.Denom, supply.Amount)}, nil
return &types.QuerySupplyOfResponse{Amount: sdk.NewCoin(req.Denom, supply)}, nil
}

// TotalSupply implements the Query/TotalSupplyWithoutOffset gRPC method
Expand Down
9 changes: 1 addition & 8 deletions x/bank/keeper/grpc_query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ func (suite *IntegrationTestSuite) TestQueryAllBalances() {
}
req = types.NewQueryAllBalancesRequest(addr, pageReq)
res, err = queryClient.AllBalances(gocontext.Background(), req)
suite.Require().NoError(err)
suite.Equal(res.Balances.Len(), 1)
suite.Nil(res.Pagination.NextKey)
}
Expand Down Expand Up @@ -149,14 +150,6 @@ func (suite *IntegrationTestSuite) TestQueryTotalSupplyOf() {
suite.Require().NotNil(res2)

suite.Require().Equal(test1Supply, res2.Amount)

// try to make SupplyWithOffset negative, should return as 0
app.BankKeeper.AddSupplyOffset(ctx, "test1", sdk.NewInt(-100000000000))
res, err = queryClient.SupplyOf(gocontext.Background(), &types.QuerySupplyOfRequest{Denom: test1Supply.Denom})
suite.Require().NoError(err)
suite.Require().NotNil(res)

suite.Require().Equal(sdk.NewInt64Coin("test1", 0), res.Amount)
}

func (suite *IntegrationTestSuite) TestQueryParams() {
Expand Down
2 changes: 1 addition & 1 deletion x/bank/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this change? I think we might want to maintain compatibility with APIs in the upstream

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea, this PR introduced tests and logic for negative supply due to offset, and when brought into osmosis causes test failures.

Copy link
Member Author

@czarcas7ic czarcas7ic Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should I just refactor the test to require 0 on an expected negative supply result? I can do this it just seemed...wrong

Copy link
Member

@p0mvn p0mvn Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense! I will close this PR and modify the test osmosis side. Thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

GetSupplyWithOffset(ctx sdk.Context, denom string) sdk.Int
GetPaginatedTotalSupplyWithOffsets(ctx sdk.Context, pagination *query.PageRequest) (sdk.Coins, *query.PageResponse, error)
IterateTotalSupplyWithOffsets(ctx sdk.Context, cb func(sdk.Coin) bool)
GetBaseDenom(ctx sdk.Context, denom string) (string, bool)
Expand Down
3 changes: 2 additions & 1 deletion x/bank/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,7 @@ func (suite *IntegrationTestSuite) TestSupply_BurnCoins() {
Require().
NoError(keeper.MintCoins(ctx, authtypes.Minter, initCoins))
supplyAfterInflation, _, err := keeper.GetPaginatedTotalSupply(ctx, &query.PageRequest{})
suite.Require().NoError(err)

suite.Require().Panics(func() { keeper.BurnCoins(ctx, "", initCoins) }, "no module account") // nolint:errcheck
suite.Require().Panics(func() { keeper.BurnCoins(ctx, authtypes.Minter, initCoins) }, "invalid permission") // nolint:errcheck
Expand All @@ -313,8 +314,8 @@ func (suite *IntegrationTestSuite) TestSupply_BurnCoins() {
authKeeper.SetModuleAccount(ctx, multiPermAcc)

err = keeper.BurnCoins(ctx, multiPermAcc.GetName(), initCoins)
supplyAfterBurn, _, err = keeper.GetPaginatedTotalSupply(ctx, &query.PageRequest{})
suite.Require().NoError(err)
supplyAfterBurn, _, err = keeper.GetPaginatedTotalSupply(ctx, &query.PageRequest{})
suite.Require().NoError(err)
suite.Require().Equal(sdk.NewCoins().String(), getCoinsByName(ctx, keeper, authKeeper, multiPermAcc.GetName()).String())
suite.Require().Equal(supplyAfterInflation.Sub(initCoins), supplyAfterBurn)
Expand Down
9 changes: 2 additions & 7 deletions x/bank/keeper/supply_offset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,10 @@ func (k BaseKeeper) AddSupplyOffset(ctx sdk.Context, denom string, offsetAmount
// GetSupplyWithOffset retrieves the Supply of a denom and offsets it by SupplyOffset
// If SupplyWithOffset is negative, it returns 0. This is because sdk.Coin is not valid
// with a negative amount
func (k BaseKeeper) GetSupplyWithOffset(ctx sdk.Context, denom string) sdk.Coin {
func (k BaseKeeper) GetSupplyWithOffset(ctx sdk.Context, denom string) sdk.Int {
supply := k.GetSupply(ctx, denom)
supply.Amount = supply.Amount.Add(k.GetSupplyOffset(ctx, denom))

if supply.Amount.IsNegative() {
supply.Amount = sdk.ZeroInt()
}

return supply
return supply.Amount
}

// GetPaginatedTotalSupplyWithOffsets queries for the supply with offset, ignoring 0 coins, with a given pagination
Expand Down