feat: enable whitelist stableswap #289
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the transaction handling and query capabilities of the application. Key modifications include the introduction of new gRPC services for querying gas prices, improvements to the transaction processing structure, and the addition of a new Changes
Possibly related PRs
Poem
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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (29)
x/move/keeper/voting_power.go (1)
54-54: Consistent use of MoveBankKeeper instance.This change is good and consistent with the previous modification. It eliminates the need to create a new
MoveBankKeeperinstance, improving efficiency.For improved readability, consider adding a comment explaining the purpose of
bk, especially if it's used in multiple places within the function. For example:// bk is the MoveBankKeeper instance used for balance and supply queries bk := k.MoveBankKeeper()x/move/keeper/voting_power_test.go (1)
68-69: LGTM: Updated voting power weight for stable swap poolsThe change in the expected voting power weight for stable swap LP tokens (now
math.LegacyOneDec().QuoInt64(3)) correctly reflects the equal weighting of the three tokens in the stable swap pool. This is consistent with the stable swap concept where all assets are considered to have equal value.Consider adding a brief comment explaining the rationale behind this calculation for future maintainers.
sdk.NewDecCoin(bondDenom, math.NewInt(1)), - // only locked base coin amount is considered + // For stable swap pools, each token contributes equally to the voting power sdk.NewDecCoinFromDec(denomLP, math.LegacyOneDec().QuoInt64(3))),x/move/keeper/api.go (1)
Line range hint
98-98: Create an issue for future error handling improvementThe TODO comment indicates a planned change from panic to error handling. While this is a good intention, it's important to track this improvement formally.
Would you like me to create a GitHub issue to track the task of changing the error handling from panic to returning an error in the
UnbondTimestampfunction?x/move/keeper/stableswap_test.go (5)
16-74: LGTM! Consider adding a comment for the magic number.The
createStableSwapPoolfunction is well-structured and correctly sets up a StableSwap pool for testing purposes. The error handling is appropriate, and the use ofExecuteEntryFunctionfor pool creation is correct.Consider adding a comment to explain the significance of the magic number 3000 on line 52:
+ // 3000 represents the amplification coefficient (A) for the StableSwap pool annBz, err := vmtypes.SerializeUint64(3000)
76-117: LGTM! Consider adding a negative test case.The
Test_StableSwap_HasPoolfunction effectively tests the creation and existence of a StableSwap pool. It correctly verifies the pool's existence, checks for non-existent pools, and validates the pool metadata.To improve test coverage, consider adding a negative test case for
GetPoolMetadatawith a non-existent pool:nonExistentPool := types.NamedObjectAddress(vmtypes.TestAddress, "nonexistent") _, err = stableSwapKeeper.GetPoolMetadata(ctx, nonExistentPool) require.Error(t, err)
119-140: LGTM! Consider enhancing the test assertions.The
Test_StableSwap_Whitelistfunction correctly tests the basic functionality of whitelisting a StableSwap pool. However, the test could be more comprehensive.Consider enhancing the test with the following improvements:
Add an assertion to verify that the pool is actually whitelisted after calling the
Whitelistmethod. This might involve adding a method to check the whitelist status of a pool.Test whitelisting the same pool twice to ensure idempotency.
Add a test case for whitelisting a non-existent pool to verify error handling.
Example:
// Verify the pool is whitelisted isWhitelisted, err := stableSwapKeeper.IsWhitelisted(ctx, metadataLP) require.NoError(t, err) require.True(t, isWhitelisted) // Test idempotency err = stableSwapKeeper.Whitelist(ctx, metadataLP) require.NoError(t, err) // Test whitelisting non-existent pool nonExistentPool := types.NamedObjectAddress(vmtypes.TestAddress, "nonexistent") err = stableSwapKeeper.Whitelist(ctx, nonExistentPool) require.Error(t, err)
142-163: LGTM! Consider adding an assertion for the specific error type.The
Test_StableSwap_Whitelist_Failed_MissingBasefunction effectively tests the error handling when attempting to whitelist a pool with a missing base denomination. The test setup and execution are correct.To improve the test's clarity and specificity, consider asserting the exact error type or message. This ensures that the correct error is being thrown for the right reason. For example:
err := stableSwapKeeper.Whitelist(ctx, metadataLP) require.Error(t, err) require.Contains(t, err.Error(), "missing base denomination") // Or, if you have specific error types: // require.ErrorIs(t, err, types.ErrMissingBaseDenomination)
1-163: Overall, the test suite is well-structured and covers key functionalities.The
stableswap_test.gofile provides a solid foundation for testing the StableSwap functionality. The tests cover essential aspects such as pool creation, existence checks, and whitelisting. The code is well-organized and follows good testing practices.To further enhance the test suite, consider the following suggestions:
- Add more edge cases and boundary conditions to increase test coverage.
- Implement table-driven tests for scenarios that can be parameterized.
- Consider adding benchmarks for performance-critical operations.
- Ensure consistent error handling and error type assertions across all tests.
- Add tests for any concurrent operations if applicable to the StableSwap functionality.
These improvements will help ensure the robustness and reliability of the StableSwap implementation.
x/move/keeper/balancer_test.go (4)
52-82: LGTM: Test_ReadWeights is well-implemented, with a minor suggestion.The test effectively verifies the pool weights after setting up a DEX pool. The assertions are correct and use
requirefor clear error reporting.For consistency with other tests, consider using
input.MoveKeeper.DexKeeper()instead ofkeeper.NewDexKeeper(&input.MoveKeeper)on line 54. This aligns with the approach used in other test functions.
84-113: LGTM: Test_GetBaseSpotPrice is well-implemented, with a suggestion for enhancement.The test effectively verifies the base spot price after setting up a DEX pool. The assertions are correct and use
requirefor clear error reporting.Consider adding more test cases with different pool configurations to ensure the
GetBaseSpotPricefunction works correctly under various scenarios. This could include:
- A pool with unequal weights
- A pool with very large or very small balances
- Edge cases where the spot price might be affected
These additional test cases would increase the robustness of the test suite.
115-150: LGTM: Test_SwapToBase is well-implemented, with suggestions for enhancement.The test effectively sets up a DEX pool, performs a swap, and verifies the resulting balance. The assertions are correct and account for the swap fee.
Consider enhancing the test with the following additions:
- Verify the pool balances after the swap to ensure they've been updated correctly.
- Add assertions to check that the quote balance is zero after the swap.
- Consider adding a test case for a partial swap where some quote tokens remain.
- Verify that the swap event is emitted correctly, if applicable.
These additions would provide a more comprehensive test of the swap functionality.
1-150: Overall, the test file is well-structured and covers key functionalities.The test file provides good coverage of the basic balancer functionalities, including reading pool balances, weights, getting base spot price, and performing swaps. The tests are well-organized and use appropriate assertions.
To further enhance the test suite, consider the following improvements:
- Add more edge cases and diverse scenarios for each test function.
- Implement property-based testing for functions like
SwapToBaseto cover a wider range of inputs.- Add tests for error cases, such as insufficient liquidity or invalid inputs.
- Ensure consistent usage of
input.MoveKeeper.DexKeeper()across all test functions.- Add tests for any untested functions in the balancer keeper, if any exist.
These enhancements would increase the robustness and coverage of the test suite, helping to catch potential edge cases and ensure the reliability of the balancer functionality.
x/move/keeper/handler.go (2)
Line range hint
1-690: Consider performance and compatibility in JSON argument transitionThe shift from byte slice arguments to JSON in
ExecuteEntryFunctionJSON,ExecuteScriptJSON, andExecuteViewFunctionJSONimproves API usability. However, consider the following:
- Performance: JSON parsing may introduce overhead. Ensure this doesn't significantly impact high-throughput scenarios.
- Backwards Compatibility: With the old methods deprecated, have you planned for a transition period or provided migration guides for existing users?
- Error Handling: Ensure robust error handling for JSON parsing failures.
Consider benchmarking the new JSON-based functions against the old byte-based ones to quantify any performance differences. This data can help in making informed decisions about the transition timeline and any necessary optimizations.
Line range hint
1-690: Summary of changes and their impactThe modifications in this file represent significant improvements:
- JSON Argument Handling: Enhances API usability but may have performance implications.
- Gas Computation: Improves system stability with better overflow protection.
- Code Organization: The deprecation of older methods in favor of JSON-based ones indicates a clear direction for future development.
These changes collectively modernize the codebase and improve its robustness. However, it's crucial to:
- Thoroughly test performance impacts, especially in high-throughput scenarios.
- Provide clear migration paths for users of the deprecated methods.
- Ensure consistent error handling across the new JSON-based methods.
Consider creating a comprehensive test suite that covers various edge cases in JSON parsing and gas computation to ensure the stability of these new changes across different scenarios.
app/app.go (2)
470-471: LGTM: New transaction query registration added.The addition of
initiatx.RegisterTxQueryenhances the transaction service by registering new query capabilities. This aligns with the PR objectives and improves the application's functionality.Consider adding a brief comment explaining the purpose of this new registration for better code documentation:
// Register custom transaction queries initiatx.RegisterTxQuery(app.GRPCQueryRouter(), app.MoveKeeper.DexKeeper())
Missing tests for transaction query functionalities
No tests were found related to the new transaction query functionalities implemented in
app/app.go. It's essential to add comprehensive tests to ensure the correct operation of these features.
- Add test cases in appropriate
*_test.gofiles to cover transaction queries.🔗 Analysis chain
Line range hint
1-594: Overall assessment: Changes enhance transaction handling and querying capabilities.The modifications in this file successfully integrate new transaction handling and querying functionalities into the InitiaApp. These changes are well-structured, follow existing patterns, and align with the PR objectives. The enhancements to the API routes and transaction services will improve the application's capabilities in handling and querying transactions.
To further improve the code:
- Consider adding brief comments for the new functionalities to enhance code documentation.
- Ensure that comprehensive tests are in place to verify the correct operation of these new transaction query routes and services.
- Update any relevant documentation to reflect these new capabilities.
To ensure all necessary tests are in place, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the presence of tests related to the new transaction query functionality # Test: Search for test files related to transaction queries echo "Searching for test files related to transaction queries..." rg -g '*_test.go' -i '(test|spec).*transaction.*query' # Test: Check for specific test cases related to the new functionality echo "Checking for specific test cases..." rg -g '*_test.go' -i '(test|spec).*(initiatx|RegisterGRPCGatewayRoutes|RegisterTxQuery)'Length of output: 355
Script:
#!/bin/bash # Description: Comprehensive search for tests related to transaction queries in the codebase # Test 1: Search for test files containing keywords related to transaction queries echo "Searching for test files containing 'transaction' and 'query' keywords..." rg -g '*_test.go' -i 'transaction.*query' # Test 2: Search for test functions related to specific functionalities echo "Searching for test functions related to 'initiatx', 'RegisterGRPCGatewayRoutes', and 'RegisterTxQuery'..." rg -g '*_test.go' -i 'func Test.*(InitiaTx|RegisterGRPCGatewayRoutes|RegisterTxQuery)' # Test 3: Search for any test files importing the related modules echo "Searching for test files importing 'initiatx', 'RegisterGRPCGatewayRoutes', or 'RegisterTxQuery'..." rg -g '*_test.go' -i 'import .*?(initiatx|RegisterGRPCGatewayRoutes|RegisterTxQuery)' # Test 4: List all test files for manual inspection echo "Listing all test files for potential manual review..." find . -type f -name '*_test.go'Length of output: 5477
x/move/types/connector.go (2)
29-29: LGTM. Consider adding documentation.The addition of
MoveModuleNameStableSwapconstant is consistent with the existing naming convention. It suggests the introduction of a new stable swap module, which aligns with the PR objectives.Consider adding a brief comment explaining the purpose of this new module, similar to other module name constants in this file.
651-677: LGTM. Consider adding error handling and documentation.The
ReadStableSwapPoolfunction is a good addition that aligns with the introduction of the stable swap module. It correctly reads various components of a stable swap pool from raw bytes.Consider the following improvements:
- Add error handling for potential out-of-bounds access when reading the byte slice.
- Add documentation comments explaining the purpose of the function and the structure of the input byte slice.
- Consider returning an error if the input byte slice is shorter than expected.
Example improvement:
// ReadStableSwapPool reads a stable swap pool configuration from raw bytes // The byte slice should contain: ExtendRef, Ann, fee rate, and metadata addresses func ReadStableSwapPool(bz []byte) ([]vmtypes.AccountAddress, error) { + if len(bz) < AddressBytesLength + 8 + 32 + 1 { // Minimum expected length + return nil, errors.New("input byte slice is too short") + } cursor := int(0) // read ExtendRef cursor += AddressBytesLength + 8 // ... (rest of the function) return metadata, nil }proto/initia/tx/v1/query.proto (1)
16-16: Clarify the documentation for theGasPricemethod.The comment for the
GasPricemethod does not specify that it returns the gas price for a specific denomination. To improve clarity, consider updating the comment to reflect this.Apply this diff to update the comment:
// GasPrice returns the gas price for the network. + // This method returns the gas price for a specific denomination.x/move/keeper/stableswap.go (1)
86-89: Implement functionality forDelistmethodThe
Delistmethod is currently a no-op and does not perform any actions. If delisting functionality is needed, consider implementing the logic to remove a stableswap pool from the whitelist or add aTODOcomment specifying that this method is pending implementation.Would you like assistance in drafting the implementation for this method or opening a GitHub issue to track this task?
x/move/keeper/dex_test.go (2)
96-98: Variabledenommay need clarification.The variable
denomis set to"foo", which might not be an intended denomination in the context of the test. Consider using a more meaningful or existing denomination to avoid confusion.Apply this diff to improve clarity:
-func TestDexPair(t *testing.T) { - ctx, input := createDefaultTestInput(t) - dexKeeper := keeper.NewDexKeeper(&input.MoveKeeper) - - denom := "foo" + denom := "uusdc" // Use a realistic denomination
Line range hint
134-173: Enhance gas price calculations and validations.The
Test_Dex_GasPricesfunction performs essential checks on gas price calculations based on DEX operations. However, consider the following improvements:
Use consistent denominations:
The
denomQuoteis set to"uusdc", but earlier tests use different denominations. Ensure consistency across tests for clarity.Apply this diff to align denominations:
-denomQuote := "uusdc" +denomQuote := "ufoo" // Example consistent denominationDetailed Assertions:
While the test checks that the
quoteGasPriceis equal tobaseGasPrice.MulInt64(4), it would be clearer to assert the exact expected value if known.Update the assertion to reflect the exact expected gas price:
require.Equal(t, baseGasPrice.MulInt64(4), quoteGasPrice.Amount) +// For example, if baseGasPrice is 0.025, then quoteGasPrice should be 0.1Check for Additional Gas Prices:
If there are more denominations involved in gas price calculations, consider extending the tests to include them.
x/move/keeper/whitelist.go (2)
29-31: Nitpick: Consistent capitalization in commentsFor consistency and clarity, consider capitalizing "dex" to "DEX" in the comment.
Apply this diff:
- // - // dex specific whitelist ops - // + // + // DEX-specific whitelist operations + //
139-142: Nitpick: Consistent capitalization in commentsFor consistency and clarity, consider capitalizing "dex" to "DEX" in the comment.
Apply this diff:
- // - // dex specific delist ops - // + // + // DEX-specific delist operations + //x/move/keeper/dex.go (1)
171-206: Optimize error handling inGasPricesmethodConsider restructuring the error handling within the
DexPairs.Walkfunction to reduce nesting and improve readability. By returning early on errors, the code becomes cleaner.Apply this diff to refactor error handling:
err = k.DexPairs.Walk(ctx, nil, func(key, value []byte) (stop bool, err error) { metadataQuote, err := vmtypes.NewAccountAddressFromBytes(key) if err != nil { return true, err } denomQuote, err := types.DenomFromMetadataAddress(ctx, k.MoveBankKeeper(), metadataQuote) if err != nil { return true, err } metadataLP, err := vmtypes.NewAccountAddressFromBytes(value) if err != nil { return true, err } baseSpotPrice, err := k.getBaseSpotPrice(ctx, metadataLP) if err != nil { return true, err } + if baseSpotPrice.IsZero() { + return true, fmt.Errorf("baseSpotPrice is zero for metadataLP %s", metadataLP.String()) + } gasPrice := baseGasPrice.Quo(baseSpotPrice) gasPrices = gasPrices.Add(sdk.NewDecCoinFromDec(denomQuote, gasPrice)) return false, nil })x/move/keeper/whitelist_test.go (1)
261-261: Typographical Error in CommentLine 261 contains a typo in the comment: "currnetly" should be "currently" and "registeration" should be "registration".
Apply this diff to correct the spelling:
- // check dex pair update (currnetly registeration itself is not performed) + // check dex pair update (currently, registration itself is not performed)x/move/keeper/balancer.go (1)
81-81: Correct the grammatical error in the comment.The comment on line 81 should read "assert base denom exists in the dex pair" for proper grammar.
Apply this diff to fix the comment:
- // assert base denom is exist in the dex pair + // assert base denom exists in the dex pairx/move/client/cli/utils.go (1)
Line range hint
276-291: Fix Incorrect Overflow Message and Byte Shifting Logic inDivideUint256StringThere are two issues in the
DivideUint256Stringfunction:
Incorrect Overflow Error Message: The error message references
Uint128instead ofUint256, which could be misleading.In-Place Modification of
nLeading to Incorrect Segment Extraction: Shiftingnin place withn.Rsh(n, 64)modifies the original value, causing subsequent extractions to yield incorrect results.Apply this diff to correct the error message and adjust the shifting logic:
func DivideUint256String(s string) (uint64, uint64, uint64, uint64, error) { n := new(big.Int) var ok bool if strings.HasPrefix(s, "0x") { _, ok = n.SetString(strings.TrimPrefix(s, "0x"), 16) } else { _, ok = n.SetString(s, 10) } if !ok { return 0, 0, 0, 0, fmt.Errorf("failed to parse %q as uint256", s) } if n.Sign() < 0 { return 0, 0, 0, 0, errors.New("value cannot be negative") } else if n.BitLen() > 256 { - return 0, 0, 0, 0, errors.New("value overflows Uint128") + return 0, 0, 0, 0, errors.New("value overflows Uint256") } - low := n.Uint64() - high := n.Rsh(n, 64).Uint64() - highLow := n.Rsh(n, 64).Uint64() - highHigh := n.Rsh(n, 64).Uint64() + low := new(big.Int).And(n, big.NewInt(0xFFFFFFFFFFFFFFFF)).Uint64() + nShifted := new(big.Int).Rsh(n, 64) + high := new(big.Int).And(nShifted, big.NewInt(0xFFFFFFFFFFFFFFFF)).Uint64() + nShifted.Rsh(nShifted, 64) + highLow := new(big.Int).And(nShifted, big.NewInt(0xFFFFFFFFFFFFFFFF)).Uint64() + nShifted.Rsh(nShifted, 64) + highHigh := new(big.Int).And(nShifted, big.NewInt(0xFFFFFFFFFFFFFFFF)).Uint64() return highHigh, highLow, high, low, nil }x/move/keeper/keeper.go (1)
174-177: Update method comments to follow Go documentation conventionsThe comments for the exported methods
DexKeeper(),MoveBankKeeper(),BalancerKeeper(), andStableSwapKeeper()should start with the method name and be written as complete sentences. This adheres to Go's best practices for documenting exported functions, enhancing readability and generating clear documentation.Apply the following diff to update the comments:
-// DexKeeper returns the dex keeper +// DexKeeper returns the DexKeeper. func (k Keeper) DexKeeper() DexKeeper { return k.dexKeeper } -// MoveBankKeeper returns the move bank keeper +// MoveBankKeeper returns the MoveBankKeeper. func (k Keeper) MoveBankKeeper() MoveBankKeeper { return k.moveBankKeeper } -// BalancerKeeper returns the balancer keeper +// BalancerKeeper returns the BalancerKeeper. func (k Keeper) BalancerKeeper() BalancerKeeper { return k.balancerKeeper } -// StableSwapKeeper returns the stable swap keeper +// StableSwapKeeper returns the StableSwapKeeper. func (k Keeper) StableSwapKeeper() StableSwapKeeper { return k.stableSwapKeeper }Also applies to: 179-182, 184-187, 189-192
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
client/docs/config.jsonis excluded by!**/*.jsonclient/docs/swagger-ui/swagger.yamlis excluded by!**/*.yamltx/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gotx/types/query.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.go
📒 Files selected for processing (26)
- app/app.go (3 hunks)
- proto/initia/tx/v1/query.proto (1 hunks)
- tx/service.go (1 hunks)
- x/move/client/cli/utils.go (2 hunks)
- x/move/common_test.go (1 hunks)
- x/move/keeper/api.go (2 hunks)
- x/move/keeper/balancer.go (1 hunks)
- x/move/keeper/balancer_test.go (1 hunks)
- x/move/keeper/bank_test.go (7 hunks)
- x/move/keeper/connector_test.go (1 hunks)
- x/move/keeper/dex.go (5 hunks)
- x/move/keeper/dex_test.go (3 hunks)
- x/move/keeper/handler.go (1 hunks)
- x/move/keeper/keeper.go (3 hunks)
- x/move/keeper/querier.go (1 hunks)
- x/move/keeper/stableswap.go (1 hunks)
- x/move/keeper/stableswap_test.go (1 hunks)
- x/move/keeper/staking.go (2 hunks)
- x/move/keeper/staking_test.go (5 hunks)
- x/move/keeper/vesting.go (1 hunks)
- x/move/keeper/voting_power.go (1 hunks)
- x/move/keeper/voting_power_test.go (1 hunks)
- x/move/keeper/whitelist.go (3 hunks)
- x/move/keeper/whitelist_test.go (3 hunks)
- x/move/types/connector.go (4 hunks)
- x/mstaking/keeper/delegation_test.go (7 hunks)
🧰 Additional context used
🪛 buf
proto/initia/tx/v1/query.proto
4-4: import "cosmos/base/v1beta1/coin.proto": file does not exist
(COMPILE)
🔇 Additional comments (57)
x/move/keeper/voting_power.go (2)
47-48: Improved efficiency and consistency in keeper usage.The changes here are a good improvement. By using
k.MoveBankKeeper()instead of creating a newDexKeeperinstance, the code becomes more efficient and aligns with the broader refactoring efforts across the codebase. This approach reduces unnecessary object creation and improves consistency in how keepers are accessed.
Line range hint
1-78: Overall improvement in code structure and efficiency.The changes in this file successfully implement the intended refactoring of the
MoveBankKeeperusage. They improve code efficiency by reducing unnecessary keeper instantiations and enhance consistency across the codebase. The core logic of theGetVotingPowerWeightsfunction remains intact, maintaining its original functionality while benefiting from a cleaner implementation.These modifications align well with the PR objectives and contribute to a more maintainable codebase. The error handling and calculation of voting power weights remain robust, ensuring the stability of the feature.
x/move/keeper/voting_power_test.go (3)
29-31: LGTM: Simplified LP token denomination derivationThe changes here streamline the test setup by directly deriving the LP token denomination from the metadata address. This approach is more straightforward and aligns well with the updated keeper structure.
41-60: LGTM: Stable swap test setup aligns with PR objectivesThe introduction of this test for stable swap pools directly addresses the PR objective of enabling whitelist stableswap functionality. The setup is comprehensive, creating a pool with three tokens, and the error handling is appropriate. This test will help ensure the correct behavior of voting power calculations for stable swap pools.
32-39: Verify the updated voting power weight calculationThe changes look good overall. The use of
VotingPowerKeeperaligns with a more modular keeper structure. However, please verify that the change in the expected voting power weight for the LP token (nowmath.LegacyNewDecWithPrec(4, 1)) accurately reflects the updated calculation logic.Also, confirm if the comment "only locked base coin amount is considered" is still applicable with the new implementation.
x/move/keeper/api.go (4)
64-66: Approved: Improved consistency in MoveBankKeeper accessThe change from
NewMoveBankKeeper(&api.Keeper)toapi.Keeper.MoveBankKeeper()simplifies the retrieval of theMoveBankKeeperinstance and aligns with similar updates across other files in the pull request. This improvement enhances code consistency and maintainability.
80-82: Approved: Consistent improvement in MoveBankKeeper accessThe change to use
api.Keeper.MoveBankKeeper()is consistent with the modification in theAmountToSharefunction. This maintains the improved code consistency and simplifies the retrieval of theMoveBankKeeperinstance.
Line range hint
134-135: Approved: Enhanced gas metering for queriesThe modification to use a normal gas meter with a maximum gas limit based on the provided
gasBalanceis a significant improvement. This change ensures accurate gas consumption tracking during query execution and prevents potential DoS attacks by respecting the gas balance. It aligns well with the overall enhancements in transaction handling and query capabilities.
Line range hint
1-165: Overall improvements in code consistency and gas meteringThe changes in this file enhance code consistency by standardizing the way
MoveBankKeeperis accessed across different functions. Additionally, the improvement in gas metering for queries adds a layer of security by preventing potential DoS attacks. These modifications align well with the PR objectives of enhancing transaction handling and query capabilities.A future improvement in error handling for the
UnbondTimestampfunction has been noted and should be tracked separately.x/move/keeper/vesting.go (2)
92-92: Approved: ImprovedMoveBankKeeperaccessThe change from
NewMoveBankKeeper(vk.Keeper)tovk.Keeper.MoveBankKeeper()is a good refactoring. It improves consistency across the codebase and likely enhances efficiency by directly accessing theMoveBankKeeperthrough theKeeperstruct instead of creating a new instance.
92-92: Verify consistency across the codebaseThe change to
vk.Keeper.MoveBankKeeper()appears to be part of a broader refactoring effort mentioned in the AI summary. To ensure consistency, it's important to verify that similar changes have been made in other relevant files.Run the following script to check for consistent usage of
MoveBankKeeper():✅ Verification successful
Consistency Confirmed Across Codebase
The usage of
MoveBankKeeper()is consistent across all relevant files in the codebase, aligning with the broader refactoring efforts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of MoveBankKeeper() across the codebase # Test: Search for MoveBankKeeper usage rg -A 5 'MoveBankKeeper\(\)' # Test: Search for any remaining instances of NewMoveBankKeeper rg 'NewMoveBankKeeper'Length of output: 16688
x/move/keeper/balancer_test.go (1)
13-50: LGTM: Test_ReadPool implementation is comprehensive and well-structured.The test covers essential aspects of pool creation and balance verification. It correctly sets up the test environment, creates a DEX pool, and verifies both pool balances and total share balance. The use of
requireensures clear error messages in case of test failures.x/move/common_test.go (1)
116-116: Approve: Good practice to clone the byte sliceThe change from
bz := val.BigInt().Bytes()tobz := slices.Clone(val.BigInt().Bytes())is a good practice. It creates a copy of the byte slice, preventing any unintended modifications to the original data. This is especially important when working with cryptographic values or when the original slice might be reused elsewhere.While there's a slight performance overhead from the copy operation, the benefits in terms of data integrity and preventing subtle bugs outweigh this cost in a test utility function.
x/move/keeper/bank_test.go (6)
43-43: LGTM: Consistent update to MoveBankKeeper instantiation.The change from
keeper.NewMoveBankKeeper(&input.MoveKeeper)toinput.MoveKeeper.MoveBankKeeper()is consistent with the previous update and aligns with the modifications mentioned in the summary.
77-77: LGTM: Consistent update to MoveBankKeeper instantiation.The change from
keeper.NewMoveBankKeeper(&input.MoveKeeper)toinput.MoveKeeper.MoveBankKeeper()is consistent with the previous updates and aligns with the modifications mentioned in the summary.
103-103: LGTM: Consistent update to MoveBankKeeper instantiation.The change from
keeper.NewMoveBankKeeper(&input.MoveKeeper)toinput.MoveKeeper.MoveBankKeeper()is consistent with the previous updates and aligns with the modifications mentioned in the summary.
139-139: LGTM: Consistent update to MoveBankKeeper instantiation.The change from
keeper.NewMoveBankKeeper(&input.MoveKeeper)toinput.MoveKeeper.MoveBankKeeper()is consistent with the previous updates and aligns with the modifications mentioned in the summary.
161-161: LGTM: Consistent update to MoveBankKeeper instantiation.The change from
keeper.NewMoveBankKeeper(&input.MoveKeeper)toinput.MoveKeeper.MoveBankKeeper()is consistent with the previous updates and aligns with the modifications mentioned in the summary.
183-183: LGTM: Consistent update to MoveBankKeeper instantiation.The change from
keeper.NewMoveBankKeeper(&input.MoveKeeper)toinput.MoveKeeper.MoveBankKeeper()is consistent with the previous updates and aligns with the modifications mentioned in the summary.Overall, these changes across all test functions in this file represent a uniform update to how the
MoveBankKeeperis accessed. This refactoring improves code consistency and aligns with the changes made in other parts of the codebase. The underlying test logic remains unchanged, which suggests that the behavior of theMoveBankKeepershould remain the same.x/move/keeper/handler.go (2)
490-492: Improved efficiency in accessing MoveBankKeeperThe change from
NewMoveBankKeeper(&k)tok.MoveBankKeeper()is a good optimization. It likely reduces unnecessary object creation, potentially improving performance, especially if this method is called frequently.
Line range hint
1-690: Improved gas computation robustnessThe changes in gas computation, particularly in
computeGasForRuntime, are well-considered:
- Overflow Protection: The check against
math.MaxUint64/gasUintScaleprevents potential overflows, enhancing system stability.- Simulation Handling: The separate gas limit for contract simulations (
k.config.ContractSimulationGasLimit) is a good practice.To ensure these changes don't introduce any regressions, consider running the following verification:
This will help identify any other areas in the codebase where similar gas computation logic might need to be updated for consistency.
x/move/keeper/staking_test.go (5)
251-251: LGTM: Refactored to use MoveBankKeeper getterThe change from
keeper.NewMoveBankKeeper(&input.MoveKeeper)toinput.MoveKeeper.MoveBankKeeper()is a good refactoring. It simplifies the code by using a getter method instead of creating a new instance, which is more efficient and consistent.
334-336: LGTM: Consistent use of MoveBankKeeper getterThe change to use
input.MoveKeeper.MoveBankKeeper()is consistent with the previous modification. This refactoring improves code consistency and reduces unnecessary object creation.
354-354: LGTM: Consistent refactoring in Test_SlashUnbondingDelegationsThe change to use
input.MoveKeeper.MoveBankKeeper()in theTest_SlashUnbondingDelegationsfunction is consistent with the previous modifications. This maintains the refactoring pattern throughout the test file.
Line range hint
438-454: LGTM: Consistent refactoring and correct slashing logicThe changes to use
input.MoveKeeper.MoveBankKeeper()are consistent with the previous modifications. Additionally, the slashing logic appears correct:
- Initial unbonding amount is 25,000,000
- After 5% slash, the new amount is 23,750,000 (which is correct: 25,000,000 * 0.95)
- The unbonding share remains unchanged at 25,000,000, which is correct as shares should not change during slashing
This implementation correctly demonstrates the effect of slashing on unbonding delegations.
Line range hint
1-454: Summary: Successful refactoring of MoveBankKeeper usageThe changes in this file consistently replace
keeper.NewMoveBankKeeper(&input.MoveKeeper)withinput.MoveKeeper.MoveBankKeeper(). This refactoring:
- Improves code consistency throughout the test file
- Potentially enhances performance by avoiding unnecessary object creation
- Maintains the correct functionality of all test cases, including the slashing logic
These changes align with good coding practices and the overall refactoring goals mentioned in the PR objectives.
app/app.go (2)
66-66: LGTM: New import for transaction handling.The addition of the
initiatximport aligns with the PR's objective of enhancing transaction handling capabilities.
443-445: LGTM: New transaction query routes registered.The addition of
initiatx.RegisterGRPCGatewayRoutesenhances the API capabilities by including new transaction query routes. This is in line with the PR objectives and improves the overall functionality of the application.x/move/types/connector.go (2)
325-325: LGTM. Comment update is accurate.The updated comment accurately reflects the changes in the function's logic, now mentioning both "extend_ref" and "version".
199-201: LGTM. Verify impact on existing usages.The changes to
DeserializeBigDecimallook good. Cloning and reversing the byte slice ensures that the original input is not modified and may be necessary for correct deserialization.Please verify that this change doesn't negatively impact existing usages of
DeserializeBigDecimal. Run the following script to find all occurrences:✅ Verification successful
Verified: No Negative Impact on Existing Usages
The changes to
DeserializeBigDecimalhave been reviewed. The function is used exclusively withinx/move/types/connector.goat the following lines:
- 198
- 331
- 339
- 354
- 362
- 458
No external usages were found, ensuring existing functionalities remain unaffected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of DeserializeBigDecimal function rg -n "DeserializeBigDecimal\(" --type goLength of output: 678
x/mstaking/keeper/delegation_test.go (6)
22-22: Improved data safety indecToVmArgumentThe use of
slices.Clonewhen converting the big.Int to bytes is a good improvement. This ensures that a copy of the byte slice is returned, preventing potential modifications to the original data and improving the function's robustness.
495-496: Improved MoveBankKeeper access inTest_DelegateThe change from
movekeeper.NewMoveBankKeeper(&input.MoveKeeper)toinput.MoveKeeper.MoveBankKeeper()is a good improvement. It standardizes the way the MoveBankKeeper is accessed, enhancing code consistency and potentially improving efficiency by avoiding the creation of a new instance.
539-540: Consistent MoveBankKeeper access inTest_UnbondThe change to
input.MoveKeeper.MoveBankKeeper()in this function maintains consistency with the previous modification inTest_Delegate. This standardized approach to accessing the MoveBankKeeper improves overall code consistency.
577-578: Consistent MoveBankKeeper access inTest_UnbondAfterSlashThe change to
input.MoveKeeper.MoveBankKeeper()in this function continues the pattern established in previous test functions. This consistency in accessing the MoveBankKeeper across different tests enhances the overall readability and maintainability of the test suite.
678-679: Completed standardization of MoveBankKeeper accessThe changes to
input.MoveKeeper.MoveBankKeeper()inTest_BeginRedelegationandTest_ValidateUnbondAmountcomplete the standardization of MoveBankKeeper access across all test functions in this file. This consistent approach enhances the readability, maintainability, and potentially the efficiency of the entire test suite.Also applies to: 723-724
22-22: Overall improvement in code consistency and safetyThe changes in this file have successfully standardized the access to MoveBankKeeper across all test functions and improved data safety in the
decToVmArgumentfunction. These modifications enhance the overall consistency, readability, and robustness of the test suite. The standardized approach to accessing MoveBankKeeper may also lead to improved efficiency. These changes are well-implemented and maintain the integrity of the tests while improving the code quality.Also applies to: 495-496, 539-540, 577-578, 678-679, 723-724
x/move/keeper/stableswap.go (2)
5-5: Verify compatibility of theslicespackageThe
slicespackage from the standard library is being imported. Ensure that the project's Go version supports this package (introduced in Go 1.21). If not, consider using alternative implementations or third-party packages.
50-56: Review error handling when the pool does not existIn the
Whitelistmethod, ifHasPoolreturnsfalse, the function returnsnil, indicating success. Is this the intended behavior? If a pool does not exist, it might be clearer to return an error to indicate that the specified pool was not found.x/move/keeper/dex_test.go (8)
19-19: Ensure data integrity with proper byte handling.Using
slices.Cloneonval.BigInt().Bytes()ensures that the original byte slice is not modified during serialization, which is a good practice to prevent unintended side effects.
99-100: Check for the existence of the DEX pair before it is set.Confirming that the DEX pair does not exist initially ensures the test starts from a clean state, which is good practice.
114-115: Verify error handling when setting invalid DEX pairs.Attempting to set a DEX pair with invalid metadata should result in an error, and the test correctly expects this behavior.
117-120: Set up valid DEX pair for further testing.Assigning valid
MetadataQuoteandMetadataLPensures that the subsequent operations have the correct context.
125-127: Confirming the DEX pair has been set successfully.After setting the DEX pair, it's good practice to verify that it now exists. The test correctly performs this check.
129-131: Validate retrieved MetadataLP matches expected value.Ensuring that the retrieved
MetadataLPis equal to the one set confirms the integrity of the store operation.
139-139: Handle potential integer overflow with large amounts.The
baseAmountis set to1_000_000_000_000, which is a large number. Ensure that the system can handle such large amounts without integer overflow issues.Run the following script to check for integer size and potential overflows:
#!/bin/bash # Description: Verify that Int types can handle large amounts without overflow. # Test: Search for instances where large amounts are used. rg --multiline --context 5 'math\.NewInt\(\d{12,}\)'This script searches for
math.NewIntinitializations with numbers of 12 or more digits to identify potential issues.
122-122: Missing check for existing DEX pair before setting it.Consider adding a check to confirm that the DEX pair does not already exist before setting it to prevent unintended overwrites.
Run the following script to check for existing DEX pairs:
This script verifies that the test includes a check for the DEX pair's existence.
x/move/keeper/whitelist.go (1)
24-27: LGTM!The code correctly converts
msg.MetadataLPintometadataLPand handles any potential errors appropriately.x/move/keeper/dex.go (3)
280-281: Verify access to unexported methods in BalancerKeeperSimilarly, in the
PoolWeightsmethod, you're callingk.BalancerKeeper().poolWeights(...). Ensure thatpoolWeightsis an exported method accessible from this context.Run the following script to check for method accessibility:
#!/bin/bash # Description: Verify that `poolWeights` is an exported method. # Expected result: The method should be exported (capitalized). rg 'func \(.*BalancerKeeper\) poolWeights' --type go
Line range hint
122-129: Confirm method receiver changes are propagatedThe receiver for
GetMetadataLPhas changed fromKeepertoDexKeeper. Ensure that all instances whereGetMetadataLPis called have been updated to use the correct receiver to prevent any compilation errors.Run the following script to find usages with the old receiver:
#!/bin/bash # Description: Find usages of `GetMetadataLP` called on `Keeper`. # Expected result: No occurrences should be found. rg 'Keeper\)\s*GetMetadataLP' --type go
268-269: Verify access to unexported methods in BalancerKeeperThe method
PoolBalancesis callingk.BalancerKeeper().poolBalances(...). Ensure thatpoolBalancesis an exported method in theBalancerKeeper. In Go, unexported methods (starting with a lowercase letter) from another package cannot be accessed externally.Run the following script to check for method accessibility:
x/move/keeper/whitelist_test.go (3)
46-47: Approved: Correct Retrieval of LP DenominationThe usage of
DenomFromMetadataAddresswithinput.MoveKeeper.MoveBankKeeper()to obtaindenomLPis appropriate and aligns with the updated method access patterns.
128-129: Approved: Consistent Retrieval of LP DenominationConsistently retrieving
denomLPusingDenomFromMetadataAddressandinput.MoveKeeper.MoveBankKeeper()ensures uniformity across test cases.
262-267: Verify Dex Pair Non-existence After DelistingLines 262-267 confirm that dex pairs for
denomCoinBanddenomCoinCdo not exist after delisting.To ensure the
HasDexPairmethod accurately reflects the delisted state, consider running the following script:This script ensures that the dex pairs are indeed not found in the keeper's state.
x/move/client/cli/utils.go (2)
208-209: Proper Cloning to Prevent Side EffectsCloning the byte slice before reversing it ensures that the original
BigIntbytes remain unmodified, preventing unintended side effects elsewhere in the code.
220-221: Consistent Handling in 'bigdecimal' CaseCloning and reversing the byte slice for
bigdecimalvalues maintains consistency and safeguards against modifying the original data.x/move/keeper/staking.go (2)
232-232: Use ofk.MoveBankKeeper()Improves Code ConsistencyUpdating to use
k.MoveBankKeeper()instead of creating a new instance withNewMoveBankKeeper(&k)enhances code consistency and maintainability across the codebase. This ensures all interactions with the Move bank keeper are centralized.
437-437: Refactored to Use SharedMoveBankKeeperInstanceChanging the balance retrieval to
k.MoveBankKeeper().Balance(ctx, unbondingCoinStore)ensures the use of the sharedMoveBankKeeperinstance. This refactoring promotes code clarity and aligns with best practices by avoiding unnecessary instantiation.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #289 +/- ##
========================================
Coverage 40.68% 40.69%
========================================
Files 267 270 +3
Lines 25432 25636 +204
========================================
+ Hits 10347 10432 +85
- Misses 13500 13596 +96
- Partials 1585 1608 +23
|
* enable whitelist stableswap * check division by zero and handle default values for balancer * remove unnecessary slices.Copy * ignore error
* support multisend * bump movevm to v0.5.1 * add missing stargate query support (#285) * fix nil memory access on authz (#281) * fix: allow to be failed with invalid message without error (#283) * allow to failed with invalid message * set reason * fix to consider movevm gas scale when we use infinity gas meter (#287) * fix to use cache context at ibc hook (#288) * feat: enable whitelist stableswap (#289) * enable whitelist stableswap * check division by zero and handle default values for balancer * remove unnecessary slices.Copy * ignore error * fix test * apply coderabbit comment * emit same events with cosmos-sdk interface * create account if not exists
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...