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

Auction proceeds burn #317

Merged
merged 4 commits into from
Oct 8, 2024
Merged

Auction proceeds burn #317

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43000b5
Initial auction burn and test
cbrit Sep 26, 2024
4604b59
Fix and improve unit testsC
cbrit Sep 27, 2024
ece8dc2
Make linter happy
cbrit Sep 27, 2024
78fa621
Add boundry tests for params
cbrit Sep 30, 2024
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
2 changes: 1 addition & 1 deletion app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ var (
cellarfeestypes.ModuleName: nil,
incentivestypes.ModuleName: nil,
axelarcorktypes.ModuleName: nil,
auctiontypes.ModuleName: nil,
auctiontypes.ModuleName: {authtypes.Burner},
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistent Burner Permission in Auction Module

The addition of {authtypes.Burner} permission to the auctiontypes.ModuleName in the maccPerms map is not supported by the current implementation. No actual burn functions are present in the auction module outside of test and mock files.

Please ensure that the burn functionality is properly implemented in the auction module or remove the burner permission to maintain consistency and security.

🔗 Analysis chain

LGTM. Verify auction module's burn logic.

The addition of {authtypes.Burner} permission to the auctiontypes.ModuleName in the maccPerms map is correct and aligns with the PR objective of implementing an "Auction proceeds burn" feature. This change allows the auction module to burn tokens.

To ensure this change is properly implemented, please verify:

  1. The auction module's logic correctly utilizes this new burning capability.
  2. There are appropriate checks and balances in place to prevent unintended token burning.

Run the following script to check for burn-related functions in the auction module:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for burn-related functions in the auction module
rg --type go -i 'func.*burn' x/auction

Length of output: 542

Script:

#!/bin/bash
# Search for burn-related functions in the auction module excluding test and mock files
rg --type go -i 'func.*burn' x/auction --glob '!*/test*/*' --glob '!*_mocks.go'

Length of output: 298

pubsubtypes.ModuleName: nil,
}

Expand Down
37 changes: 36 additions & 1 deletion integration_tests/auction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ func (s *IntegrationTestSuite) TestAuction() {
s.Require().NoError(err)
auctionQueryClient := types.NewQueryClient(val0ClientCtx)

// Query the total supply of usomm before the auction
bankQueryClient := banktypes.NewQueryClient(val0ClientCtx)
supplyRes, err := bankQueryClient.SupplyOf(context.Background(), &banktypes.QuerySupplyOfRequest{Denom: testDenom})
s.Require().NoError(err)

// Verify auction created for testing exists
auctionQuery := types.QueryActiveAuctionRequest{
AuctionId: uint32(1),
Expand Down Expand Up @@ -115,7 +120,6 @@ func (s *IntegrationTestSuite) TestAuction() {
}, time.Second*30, time.Second*5, "proposal was never accepted")
s.T().Log("Proposal approved!")

bankQueryClient := banktypes.NewQueryClient(val0ClientCtx)
balanceRes, err := bankQueryClient.AllBalances(context.Background(), &banktypes.QueryAllBalancesRequest{Address: authtypes.NewModuleAddress(types.ModuleName).String()})
s.Require().NoError(err)
s.T().Logf("Auction module token balances before bids %v", balanceRes.Balances)
Expand Down Expand Up @@ -293,6 +297,37 @@ func (s *IntegrationTestSuite) TestAuction() {
s.Require().Equal(expectedEndedAuction, *endedAuctionResponse.Auction)
s.T().Log("Ended auction stored correctly!")

s.T().Log("Verifying cellarfees module account balance...")
cellarfeesModuleAddress := authtypes.NewModuleAddress(cellarfees.ModuleName).String()
cellarfeesBalanceRes, err := bankQueryClient.AllBalances(context.Background(), &banktypes.QueryAllBalancesRequest{Address: cellarfeesModuleAddress})
s.Require().NoError(err)

totalSommPaid := expectedBid1.TotalUsommPaid.Amount.Add(expectedBid2.TotalUsommPaid.Amount)
// default burn rate is 0.5 so 50% of the SOMM paid to the cellarfees module account
expectedCellarfeesSomm := totalSommPaid.Quo(sdk.NewInt(2))

found, cellarfeesSommBalance := balanceOfDenom(cellarfeesBalanceRes.Balances, testDenom)
s.Require().True(found, "SOMM balance not present in cellarfees module account")
s.Require().GreaterOrEqual(expectedCellarfeesSomm.Int64(), cellarfeesSommBalance.Amount.Int64(), "Cellarfees module account should have 50% or less of the SOMM received from the auction (less if distributions have occurred)")
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct the assertion logic in balance comparison

In the assertion:

s.Require().GreaterOrEqual(expectedCellarfeesSomm.Int64(), cellarfeesSommBalance.Amount.Int64(), "Cellarfees module account should have 50% or less of the SOMM received from the auction (less if distributions have occurred)")

The logic is inverted. Since the cellarfees module account should have 50% or less of the SOMM received from the auction, the assertion should check that the actual balance is less than or equal to the expected balance. Consider changing the assertion to:

s.Require().LessOrEqual(cellarfeesSommBalance.Amount.Int64(), expectedCellarfeesSomm.Int64(), "Cellarfees module account should have 50% or less of the SOMM received from the auction (less if distributions have occurred)")

Apply this diff to correct the assertion:

-s.Require().GreaterOrEqual(expectedCellarfeesSomm.Int64(), cellarfeesSommBalance.Amount.Int64(), "Cellarfees module account should have 50% or less of the SOMM received from the auction (less if distributions have occurred)")
+s.Require().LessOrEqual(cellarfeesSommBalance.Amount.Int64(), expectedCellarfeesSomm.Int64(), "Cellarfees module account should have 50% or less of the SOMM received from the auction (less if distributions have occurred)")

s.T().Log("Cellarfees module account balance verified correctly!")

s.T().Log("Verifying total supply of usomm has been reduced...")

// Calculate the expected burn amount (50% of total SOMM paid in auction)
expectedBurnAmount := totalSommPaid.Quo(sdk.NewInt(2))

// Calculate the expected new total supply
expectedNewSupply := supplyRes.Amount.Amount.Sub(expectedBurnAmount)

// Query the actual new total supply
newSupplyRes, err := bankQueryClient.SupplyOf(context.Background(), &banktypes.QuerySupplyOfRequest{Denom: testDenom})
s.Require().NoError(err)

// Verify that the new supply matches the expected new supply
s.Require().Equal(expectedNewSupply.Int64(), newSupplyRes.Amount.Amount.Int64(), "Total supply of usomm should be reduced by the amount burnt")

s.T().Log("Total supply of usomm has been correctly reduced!")

s.T().Log("--Test completed successfully--")
})
}
Expand Down
1 change: 1 addition & 0 deletions integration_tests/setup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,7 @@ func (s *IntegrationTestSuite) initGenesis() {
// Add an auction for integration testing of the auction module
var auctionGenState auctiontypes.GenesisState
s.Require().NoError(cdc.UnmarshalJSON(appGenState[auctiontypes.ModuleName], &auctionGenState))
auctionGenState.Params = auctiontypes.DefaultParams()
auctionGenState.TokenPrices = append(auctionGenState.TokenPrices, &auctiontypes.TokenPrice{
Denom: testDenom,
Exponent: 6,
Expand Down
4 changes: 4 additions & 0 deletions proto/auction/v1/genesis.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,8 @@ message Params {
(gogoproto.nullable) = false
];
uint64 minimum_auction_height = 6;
string auction_burn_rate = 7 [
(gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec",
(gogoproto.nullable) = false
];
}
8 changes: 4 additions & 4 deletions proto/buf.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ deps:
- remote: buf.build
owner: cosmos
repository: cosmos-proto
commit: 1935555c206d4afb9e94615dfd0fad31
digest: shake256:c74d91a3ac7ae07d579e90eee33abf9b29664047ac8816500cf22c081fec0d72d62c89ce0bebafc1f6fec7aa5315be72606717740ca95007248425102c365377
commit: 04467658e59e44bbb22fe568206e1f70
digest: shake256:73a640bd60e0c523b0f8237ff34eab67c45a38b64bbbde1d80224819d272dbf316ac183526bd245f994af6608b025f5130483d0133c5edd385531326b5990466
- remote: buf.build
owner: cosmos
repository: cosmos-sdk
Expand All @@ -19,5 +19,5 @@ deps:
- remote: buf.build
owner: googleapis
repository: googleapis
commit: 4ed3bc159a8b4ac68fe253218760d035
digest: shake256:7149cf5e9955c692d381e557830555d4e93f205a0f1b8e2dfdae46d029369aa3fc1980e35df0d310f7cc3b622f93e19ad276769a283a967dd3065ddfd3a40e13
commit: e7f8d366f5264595bcc4cd4139af9973
digest: shake256:e5e5f1c12f82e028ea696faa43b4f9dc6258a6d1226282962a8c8b282e10946281d815884f574bd279ebd9cd7588629beb3db17b892af6c33b56f92f8f67f509
22 changes: 18 additions & 4 deletions x/auction/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,11 +325,25 @@ func (k Keeper) FinishAuction(ctx sdk.Context, auction *types.Auction) error {
usommProceeds = usommProceeds.Add(bid.TotalUsommPaid.Amount)
}

usommProceedsCoin := sdk.NewCoin(params.BaseCoinUnit, usommProceeds)

// Send proceeds to their appropriate destination module
// Send proceeds to their appropriate destination
if usommProceeds.IsPositive() {
if err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, auction.ProceedsModuleAccount, sdk.Coins{usommProceedsCoin}); err != nil {
burnRate := k.GetParamSet(ctx).AuctionBurnRate
finalUsommProceeds := usommProceeds

if burnRate.GT(sdk.ZeroDec()) {
// Burn portion of USOMM in proceeds
auctionBurn := sdk.NewDecFromInt(usommProceeds).Mul(burnRate).TruncateInt()
auctionBurnCoins := sdk.NewCoin(params.BaseCoinUnit, auctionBurn)
if err := k.bankKeeper.BurnCoins(ctx, types.ModuleName, sdk.NewCoins(auctionBurnCoins)); err != nil {
return err
}

finalUsommProceeds = finalUsommProceeds.Sub(auctionBurn)
}

// Send remaining USOMM to proceeds module
finalUsommProceedsCoin := sdk.NewCoin(params.BaseCoinUnit, finalUsommProceeds)
if err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, auction.ProceedsModuleAccount, sdk.Coins{finalUsommProceedsCoin}); err != nil {
return err
}
}
Expand Down
6 changes: 4 additions & 2 deletions x/auction/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,10 @@ func (suite *KeeperTestSuite) TestHappyPathFinishAuction() {
TotalUsommPaid: amountPaid2,
})

// Second transfer to return proceeds from bids
totalUsommExpected := sdk.NewCoin(params.BaseCoinUnit, amountPaid1.Amount.Add(amountPaid2.Amount))
// Burn and send remaining proceeds from bids
totalBurnExpected := sdk.NewCoin(params.BaseCoinUnit, amountPaid1.Amount.Add(amountPaid2.Amount).Quo(sdk.NewInt(2)))
suite.mockBurnCoins(ctx, auctionTypes.ModuleName, sdk.NewCoins(totalBurnExpected))
totalUsommExpected := sdk.NewCoin(params.BaseCoinUnit, amountPaid1.Amount.Add(amountPaid2.Amount).Sub(totalBurnExpected.Amount))
Comment on lines +220 to +222
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential integer division rounding issue in burn calculation

The calculation of totalBurnExpected uses integer division with Quo(sdk.NewInt(2)), which truncates any fractional amounts when the total is odd. This could result in under-burning the proceeds. Consider using precise decimal division to ensure accurate calculations and that exactly half of the proceeds are burned regardless of whether the total amount is even or odd.

Apply this diff to fix the rounding issue:

-	totalBurnExpected := sdk.NewCoin(params.BaseCoinUnit, amountPaid1.Amount.Add(amountPaid2.Amount).Quo(sdk.NewInt(2)))
+	totalAmount := amountPaid1.Amount.Add(amountPaid2.Amount)
+	totalBurnDec := sdk.NewDecFromInt(totalAmount).Quo(sdk.NewDec(2))
+	totalBurnExpected := sdk.NewCoin(params.BaseCoinUnit, totalBurnDec.TruncateInt())

Update the calculation of totalUsommExpected accordingly:

-	totalUsommExpected := sdk.NewCoin(params.BaseCoinUnit, amountPaid1.Amount.Add(amountPaid2.Amount).Sub(totalBurnExpected.Amount))
+	totalUsommExpected := sdk.NewCoin(params.BaseCoinUnit, totalAmount.Sub(totalBurnExpected.Amount))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
totalBurnExpected := sdk.NewCoin(params.BaseCoinUnit, amountPaid1.Amount.Add(amountPaid2.Amount).Quo(sdk.NewInt(2)))
suite.mockBurnCoins(ctx, auctionTypes.ModuleName, sdk.NewCoins(totalBurnExpected))
totalUsommExpected := sdk.NewCoin(params.BaseCoinUnit, amountPaid1.Amount.Add(amountPaid2.Amount).Sub(totalBurnExpected.Amount))
totalAmount := amountPaid1.Amount.Add(amountPaid2.Amount)
totalBurnDec := sdk.NewDecFromInt(totalAmount).Quo(sdk.NewDec(2))
totalBurnExpected := sdk.NewCoin(params.BaseCoinUnit, totalBurnDec.TruncateInt())
suite.mockBurnCoins(ctx, auctionTypes.ModuleName, sdk.NewCoins(totalBurnExpected))
totalUsommExpected := sdk.NewCoin(params.BaseCoinUnit, totalAmount.Sub(totalBurnExpected.Amount))

suite.mockSendCoinsFromModuleToModule(ctx, auctionTypes.ModuleName, permissionedReciever.GetName(), sdk.NewCoins(totalUsommExpected))

// Change active auction tokens remaining before finishing auction to pretend tokens were sold
Expand Down
4 changes: 4 additions & 0 deletions x/auction/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,11 @@ func (suite *KeeperTestSuite) TestHappyPathSubmitBidAndFulfillFully() {

// Mock out final keeper calls necessary to finish the auction due to bid draining the available supply
suite.mockGetBalance(ctx, authtypes.NewModuleAddress(auctionTypes.ModuleName), saleToken, sdk.NewCoin(saleToken, sdk.NewInt(0)))

totalUsommExpected := sdk.NewCoin(params.BaseCoinUnit, sdk.NewInt(20000000000))
totalBurnExpected := sdk.NewCoin(params.BaseCoinUnit, totalUsommExpected.Amount.Quo(sdk.NewInt(2)))
totalUsommExpected = sdk.NewCoin(params.BaseCoinUnit, totalUsommExpected.Amount.Sub(totalBurnExpected.Amount))
suite.mockBurnCoins(ctx, auctionTypes.ModuleName, sdk.NewCoins(totalBurnExpected))
suite.mockSendCoinsFromModuleToModule(ctx, auctionTypes.ModuleName, permissionedReciever.GetName(), sdk.NewCoins(totalUsommExpected))

// Submit the partially fulfillable bid now
Expand Down
4 changes: 4 additions & 0 deletions x/auction/keeper/sdk_module_mocks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,7 @@ func (suite *KeeperTestSuite) mockSendCoinsFromAccountToModule(ctx sdk.Context,
func (suite *KeeperTestSuite) mockSendCoinsFromModuleToAccount(ctx sdk.Context, senderModule string, receiverAcct sdk.AccAddress, amt sdk.Coins) {
suite.bankKeeper.EXPECT().SendCoinsFromModuleToAccount(ctx, senderModule, receiverAcct, amt).Return(nil)
}

func (suite *KeeperTestSuite) mockBurnCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) {
suite.bankKeeper.EXPECT().BurnCoins(ctx, moduleName, amt).Return(nil)
}
16 changes: 15 additions & 1 deletion x/auction/testutil/expected_keepers_mocks.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions x/auction/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,5 @@ var (
ErrAuctionBelowMinimumUSDValue = errorsmod.Register(ModuleName, 45, "auction USD value below minimum")
ErrInvalidMinimumAuctionHeightParam = errorsmod.Register(ModuleName, 46, "invalid minimum auction height param")
ErrAuctionBelowMinimumHeight = errorsmod.Register(ModuleName, 47, "auction block height below minimum")
ErrInvalidAuctionBurnRateParam = errorsmod.Register(ModuleName, 48, "invalid auction burn rate param")
)
1 change: 1 addition & 0 deletions x/auction/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,5 @@ type BankKeeper interface {
GetAllBalances(ctx sdk.Context, addr sdk.AccAddress) sdk.Coins
GetBalance(ctx sdk.Context, addr sdk.AccAddress, denom string) sdk.Coin
GetDenomMetaData(ctx sdk.Context, denom string) (banktypes.Metadata, bool)
BurnCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) error
}
Loading
Loading