Auction proceeds burn #317
Conversation
WalkthroughThe changes involve modifications to the auction module, primarily focusing on the introduction of an auction burn rate parameter and the implementation of a burn mechanism for auction proceeds. This includes updates to how proceeds are managed, ensuring a portion is burned based on the configured rate. Additionally, enhancements to testing functionalities were made to validate these changes, including new error handling for invalid auction burn rate parameters. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
proto/auction/v1/genesis.proto (1)
32-35: LGTM! Consider adding documentation for the new field.The addition of the
auction_burn_ratefield to theParamsmessage is well-structured and follows proper protobuf conventions. The use of a custom decimal type and making it non-nullable are appropriate choices for this kind of parameter.Consider adding a comment above the field to describe its purpose and any constraints on its value (e.g., valid range). This would improve the self-documentation of the protobuf file. For example:
// auction_burn_rate represents the rate at which auction proceeds are burned. // It should be a decimal value between 0 and 1, inclusive. string auction_burn_rate = 7 [ (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec", (gogoproto.nullable) = false ];x/auction/keeper/sdk_module_mocks_test.go (1)
30-32: LGTM! Consider a minor improvement for consistency.The
mockBurnCoinsmethod is well-implemented and enhances the testing capabilities of the suite. It follows the established patterns in the file and correctly sets up the mock expectation for theBurnCoinsoperation.For consistency with other mock methods in this file, consider adding a comment describing the purpose of this method. Here's a suggested improvement:
+// mockBurnCoins sets up an expectation for the BurnCoins method of the bankKeeper func (suite *KeeperTestSuite) mockBurnCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) { suite.bankKeeper.EXPECT().BurnCoins(ctx, moduleName, amt).Return(nil) }This addition would maintain consistency with the documentation style of other methods in the file and improve readability.
x/auction/types/errors.go (1)
55-55: LGTM! Consider a minor improvement to the error message.The new error variable
ErrInvalidAuctionBurnRateParamis correctly implemented and consistent with other error definitions. The error code is unique and follows the existing pattern.Consider slightly modifying the error message for improved clarity:
- ErrInvalidAuctionBurnRateParam = errorsmod.Register(ModuleName, 48, "invalid auction burn rate param") + ErrInvalidAuctionBurnRateParam = errorsmod.Register(ModuleName, 48, "invalid auction burn rate parameter")This change spells out "param" to "parameter" for consistency with other error messages in the file that use the full word.
x/auction/keeper/msg_server_test.go (1)
124-128: LGTM! Consider adding a comment for clarity.The implementation of the auction proceeds burn feature looks correct. The calculations for
totalBurnExpectedand the adjustment oftotalUsommExpectedare accurate, with half of the proceeds being burned as expected.Consider adding a brief comment explaining the purpose of these calculations for better readability:
totalUsommExpected := sdk.NewCoin(params.BaseCoinUnit, sdk.NewInt(20000000000)) +// Calculate the amount to be burned (50% of total proceeds) and adjust the remaining amount 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))integration_tests/setup_test.go (1)
Line range hint
1-1000: Consider refactoring for improved maintainabilityWhile the current change is minimal and correct, this file is quite long and complex. To improve maintainability and readability in the long term, consider breaking down this file into smaller, more focused files. For example, you could separate the setup for different components (validators, orchestrators, Ethereum nodes) into individual files. This would make the codebase easier to navigate and maintain.
x/auction/types/params_test.go (1)
174-174: Remove redundant error checkThe call to
require.Error(t, err)is redundant becauserequire.ErrorIs(t, err, tc.expErr)already asserts that an error occurred and that it matches the expected error. You can safely remove the redundant check.Apply this diff to remove the redundant error assertion:
t.Run(tc.name, func(t *testing.T) { err := tc.params.ValidateBasic() - require.Error(t, err) require.ErrorIs(t, err, tc.expErr) })🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 174-174:
Using the variable on range scopetcin function literal (scopelint)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
proto/buf.lockis excluded by!**/*.lock,!**/*.lockx/auction/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (13)
- app/app.go (1 hunks)
- integration_tests/auction_test.go (2 hunks)
- integration_tests/setup_test.go (1 hunks)
- proto/auction/v1/genesis.proto (1 hunks)
- x/auction/keeper/keeper.go (1 hunks)
- x/auction/keeper/keeper_test.go (1 hunks)
- x/auction/keeper/msg_server_test.go (1 hunks)
- x/auction/keeper/sdk_module_mocks_test.go (1 hunks)
- x/auction/testutil/expected_keepers_mocks.go (2 hunks)
- x/auction/types/errors.go (1 hunks)
- x/auction/types/expected_keepers.go (1 hunks)
- x/auction/types/params.go (7 hunks)
- x/auction/types/params_test.go (5 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci-lint
x/auction/types/params_test.go
[failure] 172-172:
Using the variable on range scopetcin function literal (scopelint)
[failure] 174-174:
Using the variable on range scopetcin function literal (scopelint)
🔇 Additional comments (17)
proto/auction/v1/genesis.proto (1)
32-35: Verify related code updates for the new auction_burn_rate field.The addition of the
auction_burn_ratefield to theParamsmessage is straightforward and correct. However, to ensure full integration of this new field, please verify that the following areas have been updated accordingly:
- The corresponding Go struct in the types package
- Any functions or methods that create or process
Paramsmessages- Unit tests covering the new field
- Any CLI commands or API endpoints that might need to include this new parameter
To assist with this verification, you can run the following script:
This script will help identify where the new
auction_burn_ratefield has been incorporated across the codebase.x/auction/types/expected_keepers.go (1)
23-23: LGTM! Don't forget to update mocks.The addition of the
BurnCoinsmethod to theBankKeeperinterface is appropriate and aligns with the PR objectives for implementing auction proceeds burn functionality. The method signature is consistent with other methods in the interface and includes proper error handling.As mentioned in the file comment, please ensure that this change is reflected in the testutil's expected keeper mocks. Run the following script to verify if the mock implementations have been updated:
If the script doesn't return any results, you may need to regenerate the mocks using the command mentioned in the file comment:
mockgen -source={ABS_REPO_PATH}/peggyJV/sommelier/x/auction/types/expected_keepers.go -destination={ABS_REPO_PATH}/peggyJV/sommelier/x/auction/testutil/expected_keepers_mocks.go✅ Verification successful
Mocks Updated Successfully
The
BurnCoinsmethod has been successfully added to theBankKeeperinterface and its mock implementation inx/auction/testutil/expected_keepers_mocks.go. This aligns with the PR objectives for implementing auction proceeds burn functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the BurnCoins method has been added to the mock implementations # Test: Search for BurnCoins method in mock files rg --type go 'func \(.*\) BurnCoins\(.*\) error' x/auction/testutilLength of output: 212
x/auction/testutil/expected_keepers_mocks.go (4)
2-2: Source file path updated.The source file path has been updated, likely due to a different development environment or file structure. This change doesn't affect the functionality of the code.
76-82: LGTM:BurnCoinsmethod added toMockBankKeeper.The
BurnCoinsmethod has been correctly implemented in theMockBankKeeperstruct. It matches the expected interface for aBurnCoinsoperation and uses the mock controller to record the method call and return a mocked error. This addition allows for proper testing of code that depends on theBurnCoinsfunctionality.
84-88: LGTM:BurnCoinsmethod added toMockBankKeeperMockRecorder.The
BurnCoinsmethod has been correctly implemented in theMockBankKeeperMockRecorderstruct. It uses the standard GoMock approach withRecordCallWithMethodTypeto record method calls. This implementation allows for setting expectations on theBurnCoinsmethod calls during tests, which is crucial for thorough testing of the auction module.
76-88: Summary:BurnCoinsfunctionality added to mock objects.The changes in this file successfully implement the
BurnCoinsfunctionality in both theMockBankKeeperandMockBankKeeperMockRecorderstructs. These additions follow GoMock conventions and are consistent with the existing code structure.This enhancement will allow for more comprehensive testing of the auction module, particularly for scenarios involving coin burning. It's a valuable addition that will contribute to the robustness of the test suite.
integration_tests/setup_test.go (2)
417-417: LGTM: Default parameters set for auction genesis stateThe added line sets the
Paramsfield of theauctionGenStateto the default parameters defined in theauctiontypesmodule. This is a good practice as it ensures that the auction module starts with a known, default configuration for testing.
417-417: Verify alignment of default auction parameters with test requirementsWhile setting default parameters is a good practice, it's important to ensure that these default values align with the specific requirements of your integration tests. Please verify that the default parameters set by
auctiontypes.DefaultParams()are suitable for your test scenarios. If any specific parameters need to be different from the defaults for testing purposes, consider setting them explicitly after this line.app/app.go (1)
Line range hint
1-1134: Summary: Auction module granted token burning permissionThe only change in this file is the addition of the
Burnerpermission to the auction module. This modification is concise and targeted, aligning well with the PR objective of implementing an "Auction proceeds burn" feature. The rest of the application setup remains unchanged, which helps maintain overall system stability.To ensure full implementation of this feature:
- Verify that the auction module's logic has been updated to use this new permission correctly.
- Check for any new parameters or configurations related to token burning in the auction module.
- Ensure that appropriate tests have been added or updated to cover this new functionality.
x/auction/types/params.go (7)
17-17: Added new parameter keyKeyAuctionBurnRateThe new parameter key
KeyAuctionBurnRatehas been added correctly and follows the existing naming conventions.
36-36: Set defaultAuctionBurnRateto 50%The default value for
AuctionBurnRateis set to0.5(50%). Ensure that this default aligns with the intended economic model for the auction system.
49-49: RegisteredAuctionBurnRatein parameter set pairsThe
AuctionBurnRateparameter is properly registered inParamSetPairswith its corresponding validation functionvalidateAuctionBurnRate.
75-78: IncludedAuctionBurnRateinValidateBasicThe
AuctionBurnRateparameter is now included in theValidateBasicmethod, ensuring its value is validated during parameter checks.
110-113: Added nil check forMinimumSaleTokensUSDValueIncluding a nil check for
minimumSaleTokensUsdValueenhances the robustness of the validation by preventing potential nil dereference errors.
137-140: Added nil check forAuctionPriceDecreaseAccelerationRateAdding a nil check for
auctionPriceDecreaseAccelerationRatestrengthens the validation logic and prevents possible runtime errors due to nil values.
157-173: ImplementedvalidateAuctionBurnRatefunctionThe
validateAuctionBurnRatefunction correctly:
- Asserts the type is
sdk.Dec.- Checks for nil values.
- Validates that
auctionBurnRateis between 0 and 1 inclusive.This ensures that the
AuctionBurnRateparameter is properly validated according to the expected constraints.x/auction/keeper/keeper.go (1)
328-346: Well-implemented auction proceeds burn logicThe new logic correctly calculates and burns a portion of the USOMM proceeds based on the
AuctionBurnRateparameter, then transfers the remaining proceeds to the specified module account. Error handling is properly managed, and the proceeds are accurately adjusted.
| @@ -217,7 +217,7 @@ var ( | |||
| cellarfeestypes.ModuleName: nil, | |||
| incentivestypes.ModuleName: nil, | |||
| axelarcorktypes.ModuleName: nil, | |||
| auctiontypes.ModuleName: nil, | |||
| auctiontypes.ModuleName: {authtypes.Burner}, | |||
There was a problem hiding this comment.
💡 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:
- The auction module's logic correctly utilizes this new burning capability.
- 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
| err := tc.params.ValidateBasic() | ||
| require.Error(t, err) | ||
| require.ErrorIs(t, err, tc.expErr) |
There was a problem hiding this comment.
Fix variable capture issue in loop when using t.Run
The variable tc is being used inside a function literal within a loop. This can lead to unexpected behavior due to the same tc variable being shared across all iterations, potentially causing tests to use the wrong test case data.
Apply this diff to capture the current value of tc in each iteration:
for _, tc := range testCases {
+ tc := tc // Capture the current value of tc
t.Run(tc.name, func(t *testing.T) {
err := tc.params.ValidateBasic()
- require.Error(t, err)
require.ErrorIs(t, err, tc.expErr)
})
}Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 172-172:
Using the variable on range scopetcin function literal (scopelint)
[failure] 174-174:
Using the variable on range scopetcin function literal (scopelint)
|
|
||
| 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)") |
There was a problem hiding this comment.
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)")| 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)) |
There was a problem hiding this comment.
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.
| 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)) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
x/auction/types/params_test.go (1)
80-80: Redundant variable assignmenttc := tcIn the loop, the assignment
tc := tcis unnecessary since there is no closure capturingtcin this context. This line can be removed to simplify the code.Proposed change:
for _, tc := range testCases { - tc := tc err := tc.params.ValidateBasic() if tc.expPass { require.NoError(t, err, tc.name) require.Nil(t, err) } else { require.Error(t, err, tc.name) require.Equal(t, tc.err.Error(), err.Error()) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- x/auction/types/params_test.go (6 hunks)
🔇 Additional comments (1)
x/auction/types/params_test.go (1)
32-32: Addition ofAuctionBurnRateto test casesThe
AuctionBurnRateparameter has been appropriately added to the test cases, ensuring comprehensive validation of this new parameter.Also applies to: 45-45, 58-58, 71-71
| func TestParamsValidateBasicUnhappyPath(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| params Params | ||
| expErr error | ||
| }{ | ||
| { | ||
| name: "Invalid minimum sale tokens USD value", | ||
| params: Params{ | ||
| PriceMaxBlockAge: uint64(1000), | ||
| MinimumBidInUsomm: uint64(500), | ||
| MinimumSaleTokensUsdValue: sdk.MustNewDecFromStr("0.5"), | ||
| AuctionMaxBlockAge: uint64(100), | ||
| AuctionPriceDecreaseAccelerationRate: sdk.MustNewDecFromStr("0.1"), | ||
| AuctionBurnRate: sdk.MustNewDecFromStr("0.1"), | ||
| }, | ||
| expErr: ErrInvalidMinimumSaleTokensUSDValue, | ||
| }, | ||
| { | ||
| name: "Invalid auction burn rate (negative)", | ||
| params: Params{ | ||
| PriceMaxBlockAge: uint64(1000), | ||
| MinimumBidInUsomm: uint64(500), | ||
| MinimumSaleTokensUsdValue: sdk.MustNewDecFromStr("1.0"), | ||
| AuctionMaxBlockAge: uint64(100), | ||
| AuctionPriceDecreaseAccelerationRate: sdk.MustNewDecFromStr("0.1"), | ||
| AuctionBurnRate: sdk.MustNewDecFromStr("-0.1"), | ||
| }, | ||
| expErr: ErrInvalidAuctionBurnRateParam, | ||
| }, | ||
| { | ||
| name: "Invalid auction burn rate (greater than 1)", | ||
| params: Params{ | ||
| PriceMaxBlockAge: uint64(1000), | ||
| MinimumBidInUsomm: uint64(500), | ||
| MinimumSaleTokensUsdValue: sdk.MustNewDecFromStr("1.0"), | ||
| AuctionMaxBlockAge: uint64(100), | ||
| AuctionPriceDecreaseAccelerationRate: sdk.MustNewDecFromStr("0.1"), | ||
| AuctionBurnRate: sdk.MustNewDecFromStr("1.1"), | ||
| }, | ||
| expErr: ErrInvalidAuctionBurnRateParam, | ||
| }, | ||
| { | ||
| name: "Nil MinimumSaleTokensUsdValue", | ||
| params: Params{ | ||
| PriceMaxBlockAge: uint64(1000), | ||
| MinimumBidInUsomm: uint64(500), | ||
| MinimumSaleTokensUsdValue: sdk.Dec{}, | ||
| AuctionMaxBlockAge: uint64(100), | ||
| AuctionPriceDecreaseAccelerationRate: sdk.MustNewDecFromStr("0.1"), | ||
| AuctionBurnRate: sdk.MustNewDecFromStr("0.1"), | ||
| }, | ||
| expErr: ErrInvalidMinimumSaleTokensUSDValue, | ||
| }, | ||
| { | ||
| name: "Nil AuctionPriceDecreaseAccelerationRate", | ||
| params: Params{ | ||
| PriceMaxBlockAge: uint64(1000), | ||
| MinimumBidInUsomm: uint64(500), | ||
| MinimumSaleTokensUsdValue: sdk.MustNewDecFromStr("1.0"), | ||
| AuctionMaxBlockAge: uint64(100), | ||
| AuctionPriceDecreaseAccelerationRate: sdk.Dec{}, | ||
| AuctionBurnRate: sdk.MustNewDecFromStr("0.1"), | ||
| }, | ||
| expErr: ErrInvalidAuctionPriceDecreaseAccelerationRateParam, | ||
| }, | ||
| { | ||
| name: "Nil AuctionBurnRate", | ||
| params: Params{ | ||
| PriceMaxBlockAge: uint64(1000), | ||
| MinimumBidInUsomm: uint64(500), | ||
| MinimumSaleTokensUsdValue: sdk.MustNewDecFromStr("1.0"), | ||
| AuctionMaxBlockAge: uint64(100), | ||
| AuctionPriceDecreaseAccelerationRate: sdk.MustNewDecFromStr("0.1"), | ||
| AuctionBurnRate: sdk.Dec{}, | ||
| }, | ||
| expErr: ErrInvalidAuctionBurnRateParam, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| tc := tc | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| err := tc.params.ValidateBasic() | ||
| require.Error(t, err) | ||
| require.ErrorIs(t, err, tc.expErr) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add boundary tests for AuctionBurnRate
Currently, the test cases check for AuctionBurnRate values that are negative and greater than 1. To ensure edge cases are handled correctly, consider adding test cases for the boundary values 0 and 1.
Suggested test cases:
-
AuctionBurnRate at lower boundary (0)
{ name: "Valid auction burn rate at lower boundary (0)", params: Params{ PriceMaxBlockAge: uint64(1000), MinimumBidInUsomm: uint64(500), MinimumSaleTokensUsdValue: sdk.MustNewDecFromStr("1.0"), AuctionMaxBlockAge: uint64(100), AuctionPriceDecreaseAccelerationRate: sdk.MustNewDecFromStr("0.1"), AuctionBurnRate: sdk.MustNewDecFromStr("0.0"), }, expErr: nil, }, -
AuctionBurnRate at upper boundary (1)
{ name: "Valid auction burn rate at upper boundary (1)", params: Params{ PriceMaxBlockAge: uint64(1000), MinimumBidInUsomm: uint64(500), MinimumSaleTokensUsdValue: sdk.MustNewDecFromStr("1.0"), AuctionMaxBlockAge: uint64(100), AuctionPriceDecreaseAccelerationRate: sdk.MustNewDecFromStr("0.1"), AuctionBurnRate: sdk.MustNewDecFromStr("1.0"), }, expErr: nil, },
This will help verify that the validation correctly accepts boundary values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/auction/types/params_test.go (2)
102-127: Thorough edge case testing for AuctionBurnRateThe addition of test cases for values slightly outside the valid range (both below 0 and above 1) is excellent. This ensures that the validation logic is strict and doesn't allow any values outside the specified range.
Consider adding a comment explaining the significance of these precise values, e.g.:
// Test with values just outside the valid range to ensure strict boundary checkingThis would help other developers understand the purpose of these specific test cases.
143-231: Excellent addition of unhappy path testsThe new
TestParamsValidateBasicUnhappyPathfunction significantly improves the test coverage by explicitly testing various error scenarios. This includes invalid values forMinimumSaleTokensUsdValue, out-of-range values forAuctionBurnRate, and nil values for decimal parameters.Consider adding a test case for an
AuctionBurnRatethat is a valid number but with too many decimal places (e.g., more than the SDK typically allows). This would ensure that the validation also checks for the correct decimal precision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- x/auction/types/params_test.go (5 hunks)
🔇 Additional comments (3)
x/auction/types/params_test.go (3)
32-32: LGTM: AuctionBurnRate parameter added consistentlyThe
AuctionBurnRateparameter has been correctly added to the existing test cases with a consistent value of 0.1. This addition ensures that the new parameter is included in the validation tests.Also applies to: 45-45, 58-58, 71-71
76-101: Great addition of boundary test cases for AuctionBurnRateThe inclusion of test cases for the lower bound (0) and upper bound (1) of
AuctionBurnRateis an excellent practice. These tests ensure that the validation logic correctly handles the extreme values of the parameter range.
132-132: Good fix for the variable capture issueThe addition of
tc := tcinside the loop is an important fix. It addresses the common Go pitfall of variable capture in loop closures, ensuring that each test case uses the correct data. This change prevents potential race conditions and unexpected behavior in parallel test execution.
Summary by CodeRabbit
Release Notes
New Features
AuctionBurnRateto enhance auction parameter settings.Bug Fixes
Tests
AuctionBurnRateparameter and its constraints.