fix(emissions): add nil checks for reward distributions #2999
Conversation
lumtis
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request involve renaming variables and constants across several files to standardize naming conventions, particularly regarding the casing of "TSS". Key updates include the renaming of Changes
Suggested labels
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (11)
x/emissions/types/message_update_params_test.go (2)
32-32: Approve change with minor suggestion for consistencyThe updated error message, "block reward amount must not be negative," is more precise and aligns well with mathematical terminology. This change improves the clarity of the test case.
For consistency with Go's standard library error messages, consider using "must be" instead of "must not be". This would result in:
require.ErrorContains(t, err, "block reward amount must be non-negative")This phrasing maintains the improved precision while adhering more closely to idiomatic Go error messages.
Line range hint
1-94: Commend comprehensive test coverageThe test file demonstrates thorough coverage of the
MsgUpdateParamsfunctionality. It includes tests for:
- Invalid authority address
- Invalid parameters
- Valid cases
- Signer validation
- Message type and route checks
- Sign bytes generation
The use of sub-tests with
t.Run()enhances readability and organization. The tests utilize appropriate assertion functions from therequirepackage, which is a best practice in Go testing.Consider adding edge cases or boundary value tests if not covered elsewhere, such as:
- Maximum allowed block reward amount
- Minimum valid non-zero block reward amount
Overall, the test file exhibits high quality and adherence to Go testing conventions.
x/emissions/types/params.go (1)
107-111: Approval with minor suggestion: Enhanced robustness in slash amount validationThe addition of a nil check and the use of
v.IsNegative()significantly improve the robustness of the validation logic. The updated error messages provide more specific information about validation failures.For consistency with other error messages in this file, consider modifying the error message on line 111 to:
return fmt.Errorf("observer slash amount must not be negative")This change would align the wording with the error messages in other validation functions.
x/emissions/types/params_test.go (2)
59-59: Approve the addition of nil check with a minor suggestion.The inclusion of a nil check in the
TestValidateObserverSlashAmountfunction enhances the robustness of the test suite. This change aligns well with the PR objective of implementing nil checks to improve system stability.To further strengthen the test, consider adding an assertion to verify the specific error message returned when a nil value is provided. This would ensure that the error handling is not only present but also provides the expected feedback.
Consider adding an assertion for the error message:
err := validateObserverSlashAmount(nil) require.Error(t, err) require.Contains(t, err.Error(), "observer slash amount cannot be nil")
73-73: Approve the addition of nil check with a minor suggestion.The inclusion of a nil check in the
TestValidateBlockRewardAmountfunction enhances the test coverage and aligns with the PR objective of implementing nil checks for improved stability.To further strengthen the test, consider adding an assertion to verify the specific error message returned when a nil value is provided. This would ensure that the error handling is not only present but also provides the expected feedback.
Consider adding an assertion for the error message:
err := validateBlockRewardsAmount(nil) require.Error(t, err) require.Contains(t, err.Error(), "block reward amount cannot be nil")x/emissions/abci.go (3)
19-25: Consider extracting the magic number as a constant.The
logEach10Blocksfunction effectively reduces log frequency. However, to enhance maintainability, consider extracting the magic number '10' as a package-level constant:const logFrequencyBlocks = 10 func logEach10Blocks(message string) { if ctx.BlockHeight()%logFrequencyBlocks == 0 { ctx.Logger().Info(message) } else { ctx.Logger().Debug(message) } }This change would make it easier to adjust the logging frequency in the future if needed.
42-44: Consider simplifying the string formatting for improved readability.While the updated logging is an improvement, the string formatting can be simplified for better readability:
logEach10Blocks(fmt.Sprintf("Block rewards %s are greater than emission pool balance %s", blockRewards, emissionPoolBalance))This change removes the redundant
.String()calls, asfmt.Sprintfwill automatically call theString()method on these types.
186-191: Enhance function comment for clarity and correctness.The renaming of the function and the use of
UndistributedTSSRewardsPoolimprove consistency. However, the function comment could be enhanced:// DistributeTSSRewards transfers the allocated rewards to the Undistributed TSS Rewards Pool. // This ensures that the reserves factor is properly calculated in the next block. func DistributeTSSRewards(ctx sdk.Context, amount sdk.Int, bankKeeper types.BankKeeper) error { coin := sdk.NewCoins(sdk.NewCoin(config.BaseDenom, amount)) return bankKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, types.UndistributedTSSRewardsPool, coin) }This comment correction fixes the typo "trasferes" and clarifies the purpose of the function.
x/emissions/abci_test.go (3)
39-39: TestBeginBlocker function updated correctly.The changes in this function are consistent with the renaming efforts, improving code clarity. However, consider extracting the repeated
emissions.BeginBlocker(ctx, *k)calls into a helper function to reduce duplication and improve maintainability.Consider applying this refactor:
func runBeginBlocker(ctx sdk.Context, k *emissionskeeper.Keeper) sdk.Context { emissions.BeginBlocker(ctx, *k) return ctx.WithBlockHeight(ctx.BlockHeight() + 1) } // Usage in tests ctx = runBeginBlocker(ctx, k)Also applies to: 46-46, 68-68, 73-73, 86-86, 103-103, 109-109, 142-142, 169-169, 200-200, 202-202, 220-220, 236-236, 255-255
320-327: TestDistributeObserverRewards function significantly improved.The changes to this function greatly enhance the test coverage, including important edge cases such as nil, negative, and zero block rewards. This improvement aligns with best practices for comprehensive unit testing.
To further enhance readability and maintainability, consider grouping related test cases into subtests using
t.Run(). This approach would make it easier to identify and run specific test scenarios.Consider restructuring the tests as follows:
func TestDistributeObserverRewards(t *testing.T) { t.Run("RewardDistribution", func(t *testing.T) { // Test cases for normal reward distribution }) t.Run("EdgeCases", func(t *testing.T) { t.Run("NilBlockReward", func(t *testing.T) { // Test case for nil block reward }) t.Run("NegativeBlockReward", func(t *testing.T) { // Test case for negative block reward }) t.Run("ZeroBlockReward", func(t *testing.T) { // Test case for zero block reward }) }) }Also applies to: 331-336, 339-350, 354-359, 362-373, 377-382, 385-396, 400-405, 408-419, 423-428, 431-434, 437-449, 450-517
585-594: New setEmissionsParams function added for testing purposes.The
setEmissionsParamsfunction provides a way to set emissions parameters directly in the store without validation. While this is useful for testing scenarios, it's important to ensure this function is not used in production code.Consider adding a comment to clearly indicate that this function is intended for testing purposes only:
// setEmissionsParams sets the emissions params in the store without validation. // IMPORTANT: This function is intended for testing purposes only and should not be used in production code. func setEmissionsParams(t *testing.T, ctx sdk.Context, k emissionskeeper.Keeper, params emissionstypes.Params) { // ... (existing implementation) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- app/app.go (1 hunks)
- testutil/keeper/keeper.go (1 hunks)
- x/emissions/abci.go (3 hunks)
- x/emissions/abci_test.go (20 hunks)
- x/emissions/module.go (1 hunks)
- x/emissions/types/keys.go (2 hunks)
- x/emissions/types/message_update_params_test.go (1 hunks)
- x/emissions/types/params.go (6 hunks)
- x/emissions/types/params_test.go (3 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/app.go
- testutil/keeper/keeper.go
🧰 Additional context used
📓 Path-based instructions (7)
x/emissions/abci.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/emissions/abci_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/emissions/module.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/emissions/types/keys.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/emissions/types/message_update_params_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/emissions/types/params.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/emissions/types/params_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (10)
x/emissions/types/keys.go (1)
45-45: Approve the variable update for consistency.The update of
UndistributedTssRewardsPoolAddressto use the renamed constantUndistributedTSSRewardsPoolmaintains consistency with the earlier constant renaming. This change ensures that the module address is correctly associated with the updated constant name.To confirm this change is applied correctly and consistently, please execute the following verification script:
x/emissions/types/params.go (5)
66-68: Approval: Improved readability in negative value checkThe modification from
dec.LT(sdk.ZeroDec())todec.IsNegative()enhances code clarity while maintaining the correct logic. This change aligns with idiomatic Go practices and improves the overall readability of the validation function.
81-83: Approval: Consistent improvement in negative value checkThe alteration from
dec.LT(sdk.ZeroDec())todec.IsNegative()maintains consistency with the previous function and enhances code clarity. This change contributes to a more uniform and readable codebase.
96-98: Approval: Consistent improvement across emission percentage validationsThe modification from
dec.LT(sdk.ZeroDec())todec.IsNegative()in this function completes the consistent update across all emission percentage validation functions. This change contributes to a more uniform, readable, and maintainable codebase.
134-138: Approval: Improved validation for block rewards amountThe modifications in this function significantly enhance the robustness of the validation logic:
- The addition of a nil check prevents potential panics when dealing with uninitialized values.
- The use of
v.IsNegative()aligns with the changes made in other validation functions, promoting consistency across the codebase.- The updated error messages provide more precise information about the nature of validation failures, which will aid in debugging and error handling.
These changes collectively contribute to a more reliable and maintainable validation process for the block rewards amount.
Line range hint
1-141: Summary: Comprehensive improvements in parameter validationThe changes made to this file significantly enhance the robustness and clarity of the parameter validation logic:
- Consistent use of
IsNegative()for checking negative values across all validation functions improves readability and maintainability.- Introduction of nil checks in relevant functions prevents potential panics and aligns with the PR objectives.
- Updated error messages provide more specific information, aiding in debugging and error handling.
These modifications collectively contribute to a more stable and reliable emissions module, effectively addressing the goals outlined in the PR objectives.
x/emissions/types/params_test.go (1)
118-118: Approve the updated error message.The modification of the error message in the
TestValidatefunction for negative block reward amounts is a commendable improvement. The new message, "block reward amount must not be negative," is more precise and aligns better with the validation logic. This change enhances the clarity of error feedback, which is crucial for effective debugging and system maintenance.x/emissions/abci.go (2)
35-39: Robust nil and non-positive check for block rewards.The addition of this check enhances the function's robustness by handling edge cases where block rewards might be nil or non-positive. The use of the
logEach10Blocksfunction for logging is appropriate and consistent with the new logging strategy.
64-64: Approved: Consistent naming convention for TSS.The renaming of
DistributeTssRewardstoDistributeTSSRewardsimproves consistency in the codebase by using the correct capitalization for the acronym "TSS" (Threshold Signature Scheme). This change aligns with Go's naming conventions for acronyms.x/emissions/abci_test.go (1)
16-17: Import statements updated appropriately.The renaming of import packages aligns with the standardization efforts mentioned in the summary. This change enhances code consistency.
Also applies to: 19-19
Nope, I'm not sure we should had a general wrapper, in some case a bug where a panic happen could reflect a deeper issue where it's not safe to let the chain continue producing blocks |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2999 +/- ##
===========================================
- Coverage 66.09% 66.07% -0.02%
===========================================
Files 397 397
Lines 22332 22347 +15
===========================================
+ Hits 14760 14766 +6
- Misses 6804 6810 +6
- Partials 768 771 +3
|
Description
Summary by CodeRabbit
New Features
Bug Fixes
Documentation