e2e test network #233
e2e test network #233
Conversation
WalkthroughThis pull request introduces significant changes across multiple files in the Exocore network, primarily focusing on the genesis state configuration and network management. A new file, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Network
participant Keeper
participant Oracle
User->>Network: Send transaction
Network->>Keeper: Process transaction
Keeper->>Oracle: Validate price
Oracle-->>Keeper: Return price data
Keeper-->>Network: Confirm transaction
Network-->>User: Transaction complete
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 9
🧹 Outside diff range and nitpick comments (9)
testutil/network/genesis_data.go (1)
48-55: Remove commented code and add documentation.The implementation is correct, but could benefit from cleanup and documentation.
- Remove the commented-out empty struct literals
- Add comments explaining why default genesis states are sufficient for testing purposes
-// DefaultGenStateOperator = operatortypes.GenesisState{} +// DefaultGenStateOperator provides a basic operator genesis state for testing DefaultGenStateOperator = *operatortypes.DefaultGenesis() -// DefaultGenStateDelegation = delegationtypes.GenesisState{} +// DefaultGenStateDelegation provides a basic delegation genesis state for testing DefaultGenStateDelegation = *delegationtypes.DefaultGenesis() +// DefaultGenStateDogfood provides a basic dogfood genesis state for testing DefaultGenStateDogfood = *dogfoodtypes.DefaultGenesis()x/oracle/keeper/nonce.go (2)
131-131: Approve with minor formatting suggestion.The enhanced error message is more descriptive and helpful for debugging. However, consider improving readability with proper spacing after the colon.
- return 0, fmt.Errorf("validator for the consKey which signed the create-price tx is not included in active validator set, signer consAddr:%s", validator) + return 0, fmt.Errorf("validator for the consKey which signed the create-price tx is not included in active validator set, signer consAddr: %s", validator)
Line range hint
163-163: Consider tracking the performance TODO.There's a TODO comment about improving performance by using a composite key (validator_feederID) for nonce storage. Consider creating an issue to track this performance optimization.
Would you like me to help create a GitHub issue to track this performance optimization task?
testutil/network/network.go (3)
119-121: Consider documenting the rationale for token value changes.The token values have been significantly reduced from consensus power values. While this might be more manageable for testing, it would be helpful to document why these specific values were chosen.
Consider adding a comment explaining the reasoning:
AccountTokens: sdk.TokensFromConsensusPower(1000, evmostypes.PowerReduction), // Reduced from higher values for more manageable testing StakingTokens: sdk.TokensFromConsensusPower(500, evmostypes.PowerReduction), // Reduced to better represent realistic staking scenarios BondedTokens: sdk.TokensFromConsensusPower(200, evmostypes.PowerReduction), // Adjusted for testing different validator states
Line range hint
416-420: Consider enhancing the key seed file security.While this is test code, it's storing sensitive information (private key seed words) in plain JSON.
Consider adding a warning comment and possibly encoding the data:
+// Warning: This is for testing only. Never store private keys in plain text in production. err = WriteFile(fmt.Sprintf("%v.json", "key_seed"), clientDir, infoBz)
534-534: Remove commented-out code.This line appears to be old configuration code that's no longer needed.
-// cfg.WriteConfigFile(filepath.Join(config.RootDir, "config", "config.toml"), config)testutil/network/util.go (3)
159-159: Remove Duplicate Word in Comment.The comment at line 159 contains a duplicate word: "actually actually." This is a minor typographical error that should be corrected for clarity.
Apply this diff:
-// collectGenFiles doesn't actually actually collect gentx and fill into genesis since we use dogfood to replace staking module. +// collectGenFiles doesn't actually collect gentx and fill into genesis since we use dogfood to replace the staking module.
341-347: Correct Typos and Improve Comment Clarity.There are several typographical errors and unclear comments in this section:
- Line 341: "evry" should be "every."
- Line 343: Incomplete comment needing clarification.
- Line 346: Spelling error in "lenght" (should be "length").
Revise the comments and error messages:
-// stakingAmount, each operator opt in evry AVS with stakingAmount of every assets +// stakingAmount: each operator opts in every AVS with stakingAmount of every asset -// intial price for every asset is 1 USD +// Initial price for every asset is 1 USD ... - return DefaultGenStateOperator, fmt.Errorf("lenght of operatorAccAddresses %d should be equal to length of consPubKeys %d", len(operatorAccAddresses), len(consPubKeys)) + return DefaultGenStateOperator, fmt.Errorf("length of operatorAccAddresses %d should be equal to length of consPubKeys %d", len(operatorAccAddresses), len(consPubKeys))
414-416: Remove or Address Commented-Out Code.Lines 414-416 contain commented-out code that checks the lengths of
consPubKeysandpowers. If this code is no longer necessary, it should be removed to keep the codebase clean.If the validation is needed, uncomment and ensure it's correctly implemented. Otherwise, remove it:
- // if len(consPubKeys) != len(powers) { - // return DefaultGenStateDogfood, fmt.Errorf("length of consensusKey %d should be equal to length of powers %d", len(consPubKeys), len(powers)) - // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
testutil/network/genesis_data.go(1 hunks)testutil/network/network.go(11 hunks)testutil/network/network_test.go(0 hunks)testutil/network/util.go(6 hunks)x/oracle/keeper/nonce.go(2 hunks)
💤 Files with no reviewable changes (1)
- testutil/network/network_test.go
🔇 Additional comments (9)
testutil/network/genesis_data.go (2)
1-9: LGTM! Package and imports are well-structured.
The imports are properly organized and all dependencies are correctly referenced.
1-55: Verify test coverage for genesis states.
Let's ensure these genesis states are properly utilized in tests.
✅ Verification successful
Let me gather more information about how these genesis states are actually used in tests.
Let me check one more thing to see how these genesis states are actually used in the network setup.
Let me check one final aspect to see how these genesis states are integrated into the test network setup.
Based on the comprehensive review of the codebase, I can now provide a final response about the genesis states usage.
Genesis states are properly integrated and utilized in the test framework
The genesis states defined in genesis_data.go are actively used in the network test setup:
- They are properly initialized in
NewGenStateAssets,NewGenStateOperator,NewGenStateDogfood, andNewGenStateDelegationfunctions - The states are integrated into the test network setup through
initGenFilesfunction - The network package provides proper test utilities in
network_test.goto verify the functionality - The genesis states are used as base configurations that can be customized for different test scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of these genesis states in test files
echo "Searching for test files using these genesis states..."
rg -t go "DefaultGenState(Assets|Operator|Delegation|Dogfood)" --type go
Length of output: 3473
Script:
#!/bin/bash
# Search for files that import and use these genesis states
echo "Searching for test files importing the network package..."
rg -l "\"github.com/ExocoreNetwork/exocore/testutil/network\"" --type go
echo -e "\nSearching for actual test usage..."
rg -t go "func.*Test.*\(.*testing\.T.*\)" -A 5 -C 10 testutil/network/
Length of output: 1966
Script:
#!/bin/bash
# Check how genesis states are used in network configuration
echo "Checking network configuration usage..."
rg -t go "DefaultGenState" -B 5 -A 10 testutil/network/network.go
# Also check if there are any other test files in the modules that use these states
echo -e "\nChecking module-specific tests..."
rg -t go "DefaultGenState" x/*/types/*_test.go
Length of output: 691
Script:
#!/bin/bash
# Check the network configuration and initialization
echo "Checking network configuration and initialization..."
cat testutil/network/network.go
# Also check the util.go file which showed usage in previous results
echo -e "\nChecking utility functions..."
cat testutil/network/util.go
Length of output: 38981
x/oracle/keeper/nonce.go (1)
5-5: LGTM: Import addition is appropriate.
The fmt import is correctly added and necessary for the enhanced error message formatting.
testutil/network/network.go (5)
83-85: LGTM! Documentation improvements for token fields.
The updated comments provide better clarity about the purpose of each token field in the Config struct.
374-385: LGTM! Improved node initialization with better error handling.
The code now properly initializes node validator files and handles P2P address parsing in a more structured way.
451-454: LGTM! Removed redundant P2P URL parsing code.
Good cleanup - this code was duplicated from the earlier P2P URL parsing at lines 380-383.
526-532: LGTM! Enhanced P2P configuration with sorted persistent peers.
The implementation properly:
- Sorts peer addresses for consistent configuration
- Sets unique persistent peers for each validator
- Excludes self from the peer list
342-343: Verify the impact of directory name changes.
The node directory paths have been updated to use 'exocored' and 'exocorecli'. Ensure this aligns with the expected binary names.
✅ Verification successful
The directory names 'exocored' and 'exocorecli' are correctly aligned with the binary names used throughout the codebase.
The verification shows that:
- 'exocored' is consistently used as the main binary name across the codebase, including in:
- App configuration (
app/app.go) - Main command (
cmd/exocored/main.go) - Build configurations (
Dockerfile,Makefile) - Network scripts and configurations
- App configuration (
- While 'exocorecli' directory is created in the test network setup, it's appropriate as it represents the CLI configuration directory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these binary names are consistent across the codebase
rg -g '!*.{md,txt}' -i 'exocored|exocorecli'
Length of output: 8612
testutil/network/util.go (1)
389-395: Verify Correct Calculation of stakingValue and Power Reduction.
In the function NewGenStateOperator, at lines 389-395, the calculation of stakingValue uses sdktypes.TokensToConsensusPower, which may not produce the correct value if evmostypes.PowerReduction is not properly defined or if token values are not in the expected units.
Please ensure that stakingAmount and evmostypes.PowerReduction are correctly defined. Run the following script to verify the values:
✅ Verification successful
Let me gather more information about the PowerReduction value and its initialization.
Based on the gathered information, I can now provide the final response:
stakingValue Calculation is Correctly Implemented
The implementation is correct because:
PowerReductionis properly defined withBaseDenomUnit = 18, making it 10^18 (standard for Ethereum compatibility)sdk.DefaultPowerReductionis correctly set toevmostypes.PowerReductioninapp.go- The conversion between tokens and consensus power is consistently used across the codebase with the same
PowerReductionvalue
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the calculation of stakingValue and ensure PowerReduction is correctly applied.
# Expected: PowerReduction should be a non-zero value.
# Check the definition of PowerReduction
rg 'PowerReduction' ./ -A 2
# Search for usages of TokensToConsensusPower
rg 'TokensToConsensusPower' ./ -A 3
Length of output: 6007
Script:
#!/bin/bash
# Check the BaseDenomUnit definition and PowerReduction initialization
rg 'BaseDenomUnit.*=' ./
# Check where PowerReduction is used in the context of staking value calculations
ast-grep --pattern 'TokensFromConsensusPower($amount, $reduction)'
Length of output: 252
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (17)
x/oracle/keeper/keeper.go (2)
21-26: Add documentation for the memoryStore struct and its fields.The new struct lacks documentation explaining:
- The purpose of the memoryStore
- The difference between
agcandagcCheckTx- When and how
updatedFeederIDsis usedAdd documentation like this:
+// memoryStore holds the in-memory state for the oracle keeper type memoryStore struct { + // cs holds the cache for oracle data cs *cache.Cache + // agc holds the aggregator context for delivery transactions agc *aggregator.AggregatorContext + // agcCheckTx holds the aggregator context for check transactions agcCheckTx *aggregator.AggregatorContext + // updatedFeederIDs tracks the IDs of feeders that have been updated updatedFeederIDs []string }
37-39: Fix inconsistent and unclear documentation.The inline comment is inaccurate and contains typos:
- It mentions "four memory cache" but the struct shows different components (cache, two aggregator contexts, and feeder IDs)
- Contains a typo in "cpoies" (should be "copies")
Update the comment like this:
- // wrap all four memory cache into one pointer to track them among cpoies of Keeper (msgServer, module) + // memStore holds the in-memory state that needs to be shared across keeper copies (msgServer, module) memStore *memoryStoretestutil/network/genesis_data.go (2)
29-51: Standardize test token configurations.Both tokens use the same staking amount (5000) without explanation. Consider:
- Using constants for common values
- Adding documentation for why these specific amounts were chosen
- Making amounts different to catch potential bugs in amount handling
+const ( + // TestETHStakingAmount represents a realistic staking amount for ETH in tests + TestETHStakingAmount = 5000 + // TestNSTETHStakingAmount is intentionally different from ETH to catch bugs + TestNSTETHStakingAmount = 3000 +) Tokens: []assetstypes.StakingAssetInfo{ { // ... - StakingTotalAmount: sdkmath.NewInt(5000), + StakingTotalAmount: sdkmath.NewInt(TestETHStakingAmount), }, { // ... - StakingTotalAmount: sdkmath.NewInt(5000), + StakingTotalAmount: sdkmath.NewInt(TestNSTETHStakingAmount), }, },
53-62: Clean up commented code and document initialization choices.The file contains commented-out alternatives for genesis state initialization. Either:
- Remove the comments if they're no longer needed
- Document why empty states are preferred over default genesis
x/oracle/keeper/params.go (1)
Line range hint
33-114: Consider refactoring for better maintainabilityThe
RegisterNewTokenAndSetTokenFeederfunction is handling multiple responsibilities and could benefit from the following improvements:
- Split the function into smaller, focused functions
- Add input validation for
oInfo- Document the function's behavior and parameters
- Move magic numbers to named constants
Consider breaking down the function like this:
// validateOracleInfo validates the input parameters func (k Keeper) validateOracleInfo(oInfo *types.OracleInfo) error // findOrCreateChain returns the chain ID, creating a new chain if necessary func (k Keeper) findOrCreateChain(p types.Params, chainInfo *types.Chain) (uint64, error) // bindAssetToExistingToken attempts to bind an assetID to an existing token func (k Keeper) bindAssetToExistingToken(ctx sdk.Context, p types.Params, oInfo *types.OracleInfo, chainID uint64) error // createNewTokenAndFeeder creates a new token and its associated feeder func (k Keeper) createNewTokenAndFeeder(ctx sdk.Context, p types.Params, oInfo *types.OracleInfo, chainID uint64) errorWould you like me to provide the complete implementation for these functions?
x/oracle/keeper/keeper_suite_test.go (3)
76-79: Consider extracting duplicated parameter initializationThis parameter initialization logic is duplicated in the SetupTest method (lines 110-113). Consider extracting it into a helper method to maintain DRY principles.
+func (suite *KeeperSuite) initializeTestParams() { + p4Test := types.DefaultParams() + p4Test.TokenFeeders[1].StartBaseBlock = 1 + suite.k.SetParams(suite.ctx, p4Test) + suite.ctx = suite.ctx.WithBlockHeight(12) +}
77-77: Document the significance of StartBaseBlock modificationThe modification of
StartBaseBlockfor the second token feeder lacks documentation explaining its purpose and significance in the test context.
116-118: Consider a more descriptive function nameWhile the function implementation is correct, the name
resetSinglecould be more descriptive. Consider renaming it to better reflect its purpose, such asresetKeeperStateorresetOracleKeeper.-func resetSingle(k keeper.Keeper) { +func resetKeeperState(k keeper.Keeper) { k.ResetAggregatorContext() k.ResetCache() }x/oracle/keeper/msg_server_create_price_test.go (1)
67-68: Remove commented code and document the architectural change.The cache retrieval has been moved from the local keeper to the app's oracle keeper. While this change makes sense architecturally, please:
- Remove the commented code if it's no longer needed
- Consider adding a comment explaining why the app's oracle keeper is preferred over the local keeper instance
- c = ks.App.OracleKeeper.GetCaches() - // c = ks.ms.Keeper.GetCaches() + // Using app's oracle keeper to ensure consistent cache state across the application + c = ks.App.OracleKeeper.GetCaches()x/oracle/keeper/msg_server_create_price.go (1)
Line range hint
19-81: Consider breaking down the CreatePrice function for better maintainabilityThe function handles multiple responsibilities including:
- Gas metering
- Timestamp validation
- Price creation
- Cache management
- Event emission
- Nonce management
Consider extracting some of these into helper methods to improve readability and maintainability.
Example refactoring:
func (ms msgServer) CreatePrice(goCtx context.Context, msg *types.MsgCreatePrice) (*types.MsgCreatePriceResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) + if err := ms.setupGasMeter(ctx); err != nil { + return nil, err + } + + if err := ms.validatePriceProposal(ctx, msg); err != nil { + return nil, err + } + + newItem, caches, err := ms.processPriceCreation(ctx, msg) + if err != nil { + return nil, err + } + + if err := ms.handleCacheAndEvents(ctx, newItem, caches, msg); err != nil { + return nil, err + } return &types.MsgCreatePriceResponse{}, nil }x/oracle/keeper/msg_server_update_params_test.go (1)
169-169: LGTM! Consider adding a comment explaining the block height choice.Setting the block height is important for this test as it establishes a specific blockchain state for validating TokenFeeder updates. However, it would be helpful to document why block height 2 was specifically chosen.
Consider adding a comment like:
+ // Set block height to 2 to test TokenFeeder updates after genesis ks.ctx = ks.ctx.WithBlockHeight(2)x/oracle/module.go (1)
213-220: Improve event emission robustnessThe current implementation could be enhanced for better reliability and maintainability.
Consider these improvements:
- if feederIDs := am.keeper.GetUpdatedFeederIDs(); len(feederIDs) > 0 { + feederIDs := am.keeper.GetUpdatedFeederIDs() + // Reset early to ensure it happens even if event emission fails + am.keeper.ResetUpdatedFeederIDs() + + if len(feederIDs) > 0 { + // Validate feederIDs before joining + validFeederIDs := make([]string, 0, len(feederIDs)) + for _, id := range feederIDs { + if id != "" { + validFeederIDs = append(validFeederIDs, id) + } + } + + if len(validFeederIDs) > 0 { + feederIDsStr := strings.Join(validFeederIDs, "_") ctx.EventManager().EmitEvent(sdk.NewEvent( types.EventTypeCreatePrice, sdk.NewAttribute(types.AttributeKeyPriceUpdated, types.AttributeValuePriceUpdatedSuccess), sdk.NewAttribute(types.AttributeKeyFeederIDs, feederIDsStr), )) } - am.keeper.ResetUpdatedFeederIDs() }x/oracle/keeper/prices.go (1)
204-204: Consider consolidating cache accesses.The method accesses
k.memStore.agcmultiple times for different parameter retrievals. Consider consolidating these accesses by retrieving all needed parameters at once at the start of the method.Here's a suggested refactoring:
func (k Keeper) AppendPriceTR(ctx sdk.Context, tokenID uint64, priceTR types.PriceTimeRound) bool { nextRoundID := k.GetNextRoundID(ctx, tokenID) // This should not happen if nextRoundID != priceTR.RoundID { return false } + var p types.Params + if k.memStore.agc != nil { + p = k.memStore.agc.GetParams() + } else { + p = k.GetParams(ctx) + } store := k.getPriceTRStore(ctx, tokenID) b := k.cdc.MustMarshal(&priceTR) store.Set(types.PricesRoundKey(nextRoundID), b) - if expiredRoundID := nextRoundID - k.memStore.agc.GetParamsMaxSizePrices(); expiredRoundID > 0 { + if expiredRoundID := nextRoundID - p.MaxSizePrices; expiredRoundID > 0 { store.Delete(types.PricesRoundKey(expiredRoundID)) } roundID := k.IncreaseNextRoundID(ctx, tokenID) // update for native tokens // TODO: set hooks as a genral approach - var p types.Params - // get params from cache if exists - if k.memStore.agc != nil { - p = k.memStore.agc.GetParams() - } else { - p = k.GetParams(ctx) - } assetIDs := p.GetAssetIDsFromTokenID(tokenID) // ... rest of the methodAlso applies to: 213-214
testutil/network/util.go (4)
203-233: Improve error handling consistency.The error handling pattern is inconsistent. Some errors are returned directly while others might be silently propagated through
MustMarshalJSON.Consider wrapping the
MustMarshalJSONcalls in a try-catch block or use regularMarshalJSONwith proper error handling:-cfg.GenesisState[assetstypes.ModuleName] = cfg.Codec.MustMarshalJSON(&assetsGenState) +marshaledState, err := cfg.Codec.MarshalJSON(&assetsGenState) +if err != nil { + return fmt.Errorf("failed to marshal assets genesis state: %w", err) +} +cfg.GenesisState[assetstypes.ModuleName] = marshaledState
380-380: Replace magic number with a named constant.The value
18446744073709551615(max uint64) is hardcoded. This should be defined as a constant for better maintainability.+const MaxOptOutHeight = uint64(18446744073709551615) -OptedOutHeight: 18446744073709551615, +OptedOutHeight: MaxOptOutHeight,
415-417: Consider making epoch parameters configurable.The epoch parameters are hardcoded which might not be suitable for all test scenarios.
Consider making these parameters configurable through the function arguments:
-func NewGenStateDogfood(consPubKeys []string, stakingAmount sdkmath.Int, genStateAssets assetstypes.GenesisState) (dogfoodtypes.GenesisState, error) { +func NewGenStateDogfood(consPubKeys []string, stakingAmount sdkmath.Int, genStateAssets assetstypes.GenesisState, epochConfig *DogfoodEpochConfig) (dogfoodtypes.GenesisState, error) { // ... - DefaultGenStateDogfood.Params.EpochIdentifier = "minute" - DefaultGenStateDogfood.Params.EpochsUntilUnbonded = 5 - DefaultGenStateDogfood.Params.MinSelfDelegation = sdkmath.NewInt(100) + if epochConfig == nil { + epochConfig = DefaultDogfoodEpochConfig + } + DefaultGenStateDogfood.Params.EpochIdentifier = epochConfig.Identifier + DefaultGenStateDogfood.Params.EpochsUntilUnbonded = epochConfig.UntilUnbonded + DefaultGenStateDogfood.Params.MinSelfDelegation = epochConfig.MinSelfDelegation
293-293: Consider usingcommon.BytesToAddressfor Ethereum address conversion.Using
hexutil.Encodedirectly on the address bytes might not handle all edge cases correctly.-stakerAddrStr := hexutil.Encode(operatorAccAddress) +stakerAddrStr := common.BytesToAddress(operatorAccAddress).Hex()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
precompiles/assets/assets_test.go(0 hunks)testutil/network/genesis_data.go(1 hunks)testutil/network/network.go(10 hunks)testutil/network/network_test.go(1 hunks)testutil/network/util.go(5 hunks)x/oracle/keeper/keeper.go(3 hunks)x/oracle/keeper/keeper_suite_test.go(2 hunks)x/oracle/keeper/msg_server_create_price.go(2 hunks)x/oracle/keeper/msg_server_create_price_test.go(5 hunks)x/oracle/keeper/msg_server_update_params.go(1 hunks)x/oracle/keeper/msg_server_update_params_test.go(1 hunks)x/oracle/keeper/params.go(2 hunks)x/oracle/keeper/prices.go(4 hunks)x/oracle/keeper/single.go(4 hunks)x/oracle/module.go(3 hunks)
💤 Files with no reviewable changes (1)
- precompiles/assets/assets_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- testutil/network/network_test.go
🧰 Additional context used
🪛 GitHub Check: Run golangci-lint
testutil/network/network.go
[failure] 19-19:
ST1019: package "cosmossdk.io/math" is being imported more than once (stylecheck)
[failure] 20-20:
ST1019(related information): other import of "cosmossdk.io/math" (stylecheck)
testutil/network/util.go
[failure] 17-17:
ST1019: package "github.com/cosmos/cosmos-sdk/types" is being imported more than once (stylecheck)
[failure] 18-18:
ST1019(related information): other import of "github.com/cosmos/cosmos-sdk/types" (stylecheck)
🪛 GitHub Check: CodeQL
x/oracle/module.go
[warning] 160-160: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
[warning] 169-169: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
🔇 Additional comments (37)
x/oracle/keeper/msg_server_update_params.go (2)
Line range hint 1-71: LGTM for the rest of the implementation.
The overall structure, error handling, and parameter update logic are well-implemented with proper validation and logging.
69-69:
Avoid discarding the aggregator context result.
The result from GetAggregatorContext is being discarded with _. If this method returns important state or error information, it should be properly handled.
Let's check the implementation of GetAggregatorContext:
Consider handling the return value or document why it's safe to ignore.
x/oracle/keeper/keeper.go (2)
14-15: LGTM: Import statements are correctly organized.
The new imports for aggregator and cache packages are necessary for the memoryStore implementation.
Line range hint 21-73: Verify memory state management across transactions.
The introduction of in-memory state requires careful handling:
- When and how is the memory state reset between blocks?
- Is there a risk of state leakage between transactions?
- How is the memory state handled during chain halts or panics?
Let's check for memory state cleanup methods:
✅ Verification successful
Let me gather more context about where these reset methods are called.
Let me check one more aspect - the message handling and transaction boundaries.
Based on the gathered evidence, I can now provide a final response about the memory state management.
Memory state is properly managed with clear boundaries ✅
The codebase implements proper memory state management through:
- Explicit reset methods for each memory component (
ResetAggregatorContext,ResetCache,ResetAggregatorContextCheckTx,ResetUpdatedFeederIDs) - Separate contexts for CheckTx and DeliverTx with proper isolation (
copy4CheckTxmethods) - Cache commitment and reset mechanisms via
CommitCache - Clear memory cleanup by setting components to nil
The implementation follows best practices for state isolation between transactions and blocks, with proper cleanup mechanisms to prevent state leakage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for methods that might handle memory state cleanup
rg -A 5 "func.*[Rr]eset|func.*[Cc]lean|func.*[Cc]lear" x/oracle/keeper/
Length of output: 2373
Script:
#!/bin/bash
# Search for where reset methods are called
rg -A 3 "ResetAggregatorContext|ResetCache|ResetAggregatorContextCheckTx|ResetUpdatedFeederIDs" x/oracle/keeper/
# Search for BeginBlock/EndBlock handlers
ast-grep --pattern 'func ($k Keeper) BeginBlock$_($ctx $_) {
$$$
}'
ast-grep --pattern 'func ($k Keeper) EndBlock$_($ctx $_) {
$$$
}'
Length of output: 2214
Script:
#!/bin/bash
# Search for transaction boundary handling in msg_server
rg -A 5 "func.*DeliverTx|func.*CheckTx|func.*Commit" x/oracle/keeper/
# Look for message handlers that might use the memory store
ast-grep --pattern 'func ($s msgServer) $_($ctx $_) {
$$$
}'
Length of output: 3604
testutil/network/genesis_data.go (1)
1-10: LGTM! Clean and well-organized imports.
x/oracle/keeper/params.go (1)
111-112: Same error handling issue as above
This code segment has the same issue with discarding potential errors from GetAggregatorContext.
Apply the same error handling pattern as suggested above.
x/oracle/keeper/keeper_suite_test.go (3)
70-70: LGTM: Correctly updated resetSingle call
The update properly passes the OracleKeeper instance to match the new function signature.
104-113: LGTM: Proper initialization sequence
The initialization sequence is correct, with keeper reset followed by proper setup of the keeper, message server, and parameters.
116-118: Verify test coverage for reset functionality
Let's ensure there are tests verifying that the keeper's state is properly reset.
✅ Verification successful
Reset functionality is properly used in test setup
The reset functionality is well integrated into the test infrastructure:
resetSingleis called in key test setup points:- During test suite initialization
- In the
Reset()method ofKeeperSuite - Before keeper initialization in test setup
- The reset functions (
ResetAggregatorContextandResetCache) are properly implemented inx/oracle/keeper/single.goto clear memory store state - The reset pattern is consistently used across the test suite to ensure a clean state between tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases that verify the reset functionality
rg -l "TestReset|Test.*Reset.*State" --type go
# Search for actual usage of ResetAggregatorContext and ResetCache
ast-grep --pattern 'func ($STRUCT *$TYPE) ResetAggregatorContext()'
ast-grep --pattern 'func ($STRUCT *$TYPE) ResetCache()'
Length of output: 479
Script:
#!/bin/bash
# Let's search for test cases that might be using these reset functions
rg -B 5 -A 5 "ResetAggregatorContext|ResetCache" --type go
# Also search for any test setup functions that might be using resetSingle
rg -B 5 -A 5 "resetSingle" --type go
Length of output: 6903
x/oracle/keeper/msg_server_create_price_test.go (3)
63-63: Verify all BasedBlock values are consistently updated.
The BasedBlock value has been changed from 1 to 11 in multiple test messages. While the changes appear consistent in this file, please verify that all related test files have been updated accordingly.
#!/bin/bash
# Search for other BasedBlock occurrences in test files
rg -A 2 "BasedBlock:" "**/*_test.go"Also applies to: 85-85, 97-97
47-47: Verify block height change rationale.
The block height expectation has been increased from 2 to 12. This change appears to correlate with the BasedBlock changes from 1 to 11 in the test messages. Please confirm if this significant increase in block height is intentional and necessary for the test scenarios.
✅ Verification successful
Let me search for more context about block height usage in the tests and related files to better understand the change.
Based on the search results, I can now provide a conclusive response about the block height change:
Block height change is consistent with test setup
The block height change from 2 to 12 is intentional and consistent with the test environment setup. This is evidenced by:
- The test suite initialization in
keeper_suite_test.goexplicitly setssuite.ctx = suite.ctx.WithBlockHeight(12) - This block height is used as the standard test context across the oracle keeper tests
- The change aligns with the codebase's block height usage patterns, particularly in cache management and round sealing logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other block height dependencies
rg -A 3 "BlockHeight\(\)" x/oracle/
Length of output: 49
Script:
#!/bin/bash
# Search for block height related tests and assertions
rg -A 3 "BlockHeight\(\)" --type go x/oracle/keeper/
# Search for block height setup in test files
rg -A 3 "WithBlockHeight" --type go x/oracle/keeper/
# Look for block-related constants or variables
rg -A 3 "(blockHeight|BlockHeight)" --type go x/oracle/types/
Length of output: 7038
106-118: Verify price list structure changes and round ID sequence.
The price list structure has been significantly modified:
NextRoundIDincreased to 3- Added a new price entry with
RoundID: 1 - Changed existing entry's
RoundIDto 2
These changes suggest a modification in how rounds are tracked. Please verify:
- The round ID sequence is correct
- The empty timestamp in the new entry is intentional
✅ Verification successful
Based on the shell script results, I can now provide a final response about the price list structure changes:
Price list structure and round ID sequence are correctly implemented
The changes in the test file align with the implementation in keeper/prices.go:
- Round ID 0 is reserved as per implementation:
// 0 roundId is reserved, expect the roundid corresponds to the slice index NextRoundIDincrement from 2 to 3 is consistent with adding a new price entry- The empty timestamp in the new entry with
RoundID: 1is valid as timestamps are optional in thePriceTimeRoundstruct - Round IDs are sequential (1, 2) which matches the implementation's requirement for continuous round IDs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related round ID handling in the implementation
ast-grep --pattern 'NextRoundID = $_' x/oracle/
Length of output: 49
Script:
#!/bin/bash
# Search for NextRoundID usage in the codebase with context
rg -A 5 "NextRoundID" x/oracle/
# Search for round ID related test cases
rg -A 5 "RoundID" x/oracle/keeper/*test.go
# Search for price list related code
ast-grep --pattern 'type PriceTimeRound struct {
$$$
}'
Length of output: 23376
x/oracle/keeper/msg_server_create_price.go (2)
Line range hint 33-38: LGTM: Clean encapsulation of aggregator context access
The refactoring to access the aggregator context through ms.Keeper improves encapsulation and follows Cosmos SDK best practices.
73-77: Verify thread safety of cache operations
While the encapsulation through ms.Keeper.GetCaches() is good, ensure that the cache operations are thread-safe if multiple validators or goroutines might access them concurrently.
x/oracle/module.go (1)
166-169:
Add error handling for keeper operations in EndBlock
The current implementation lacks proper error handling for critical keeper operations that could potentially panic during consensus.
Consider adding proper error handling:
func (am AppModule) EndBlock(ctx sdk.Context, _ abci.RequestEndBlock) []abci.ValidatorUpdate {
+ logger := am.keeper.Logger(ctx)
+
+ cs, err := am.keeper.GetCaches()
+ if err != nil {
+ logger.Error("failed to get caches", "error", err, "height", ctx.BlockHeight())
+ return []abci.ValidatorUpdate{}
+ }
+ if cs == nil {
+ logger.Error("nil caches returned", "height", ctx.BlockHeight())
+ return []abci.ValidatorUpdate{}
+ }
+
validatorUpdates := am.keeper.GetValidatorUpdates(ctx)
forceSeal := false
- agc := am.keeper.GetAggregatorContext(ctx)
+
+ agc, err := am.keeper.GetAggregatorContext(ctx)
+ if err != nil {
+ logger.Error("failed to get aggregator context", "error", err, "height", ctx.BlockHeight())
+ return []abci.ValidatorUpdate{}
+ }
+ if agc == nil {
+ logger.Error("nil aggregator context returned", "height", ctx.BlockHeight())
+ return []abci.ValidatorUpdate{}
+ }Let's verify the error handling in the keeper methods:
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 169-169: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
x/oracle/keeper/prices.go (2)
60-61: LGTM! Efficient parameter caching implementation.
The cache-first approach for parameter access is well-implemented and follows Cosmos SDK best practices for performance optimization.
94-95: LGTM! Consistent caching pattern.
The implementation maintains consistency with the caching pattern used throughout the keeper.
testutil/network/util.go (1)
17-18:
Consolidate duplicate SDK type imports.
The package github.com/cosmos/cosmos-sdk/types is imported twice with different aliases (sdk and sdktypes). This can lead to confusion and inconsistency.
sdk "github.com/cosmos/cosmos-sdk/types"
-sdktypes "github.com/cosmos/cosmos-sdk/types"Update all references of sdktypes to sdk throughout the code.
Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 17-17:
ST1019: package "github.com/cosmos/cosmos-sdk/types" is being imported more than once (stylecheck)
[failure] 18-18:
ST1019(related information): other import of "github.com/cosmos/cosmos-sdk/types" (stylecheck)
x/oracle/keeper/single.go (7)
14-19: LGTM!
The GetCaches method has been updated to use the memStore field of the Keeper struct, ensuring that the cache is tied to the specific instance of Keeper. This change enhances the encapsulation of state management within the Keeper instance.
23-56: LGTM!
The GetAggregatorContext method has been refactored to access the aggregator context from the memStore field of the Keeper struct, replacing the previous global variables. The logic for initializing and resetting the aggregator context has also been modified to utilize the instance's memStore. These changes improve the encapsulation of the Keeper's state and promote a more object-oriented design.
Line range hint 59-157: LGTM!
The recacheAggregatorContext function has been converted to an instance method of the Keeper struct, allowing it to operate on the instance's state. It now uses instance methods like GetAllExocoreValidators and GetParams to manage validator powers and parameters, enhancing the encapsulation of the Keeper's state. The changes align with the overall goal of improving the object-oriented design and maintainability of the codebase.
Line range hint 160-182: LGTM!
The initAggregatorContext function has been updated to take a pointer to the Keeper struct as a parameter, allowing it to access the instance's methods directly. This change is consistent with the overall refactoring effort to enhance the encapsulation and object-oriented design of the Keeper.
184-193: LGTM!
The ResetAggregatorContext, ResetCache, and ResetAggregatorContextCheckTx methods have been modified to clear the respective fields in the memStore of the Keeper instance, rather than relying on global variables. This change ensures that the state is managed within the instance, promoting better encapsulation and maintainability.
204-215: LGTM!
The ResetUpdatedFeederIDs, GetUpdatedFeederIDs, and AppendUpdatedFeederIDs methods have been updated to operate on the updatedFeederIDs field of the memStore in the Keeper instance. This change ensures that the updated feeder IDs are managed within the instance, enhancing the encapsulation and consistency of state management.
Line range hint 1-216: Verify the impact of the changes on the codebase.
The refactoring of the Keeper struct to encapsulate state management is a significant change that affects multiple parts of the codebase. It's crucial to ensure that all the code that interacts with the Keeper has been updated accordingly.
Run the following script to identify potential areas that may require updates:
Analyze the output to identify any inconsistencies or areas that may require further updates to align with the refactored Keeper struct.
✅ Verification successful
Based on the search results, I can now generate the final response since I have comprehensive information about how the Keeper's methods and fields are used across the codebase.
The refactoring of the Keeper struct appears to be consistent across the codebase
The search results show that all interactions with the Keeper's memory store fields (memStore.cs, memStore.agc, memStore.agcCheckTx, memStore.updatedFeederIDs) are properly encapsulated through the corresponding getter and setter methods. The usage patterns found in the codebase are consistent:
- Memory store fields are only accessed directly within the Keeper's own methods in
single.go - External code only interacts with the memory store through public methods
- All reset operations are properly handled through dedicated methods
- The state management logic in
GetAggregatorContextcorrectly handles both CheckTx and non-CheckTx modes - Cache operations are properly synchronized with the aggregator context lifecycle
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description:
# Verify that the code interacting with the Keeper struct has been updated
# to use the new instance methods and fields.
# Test 1: Search for usage of the updated methods
methods=(
"GetCaches"
"GetAggregatorContext"
"recacheAggregatorContext"
"ResetAggregatorContext"
"ResetCache"
"ResetAggregatorContextCheckTx"
"ResetUpdatedFeederIDs"
"GetUpdatedFeederIDs"
"AppendUpdatedFeederIDs"
)
for method in "${methods[@]}"; do
echo "Checking usage of $method:"
rg -A 5 "\.$method"
done
# Test 2: Search for usage of the memStore fields
fields=(
"memStore.cs"
"memStore.agc"
"memStore.agcCheckTx"
"memStore.updatedFeederIDs"
)
for field in "${fields[@]}"; do
echo "Checking usage of $field:"
rg -A 5 "\.$field"
done
Length of output: 19372
testutil/network/network.go (12)
13-13: ****
The sort package import is necessary for sorting the addressesIPs slice later in the code.
82-95: LGTM!
The changes to the Config struct look good:
- The
AccountTokensfield comment is updated to clarify the token denomination. - A new
DepositedTokensfield is added. - The
StakingTokensfield comment is updated to indicate the amount for every supported asset.
The other fields and their comments remain unchanged.
240-240: LGTM!
Initializing the addressesIPs slice with the correct size (cfg.NumValidators) is the appropriate approach. This avoids unnecessary allocations and resizing of the slice later.
340-341: LGTM!
The changes to construct the nodeDir and clientDir paths using filepath.Join with the network.BaseDir and nodeDirName are correct. This ensures that the directories are created within the network's base directory and have the appropriate naming convention.
377-382: LGTM!
Parsing the p2pAddr to extract the hostname and port, and constructing the addressesIPs entries by combining the nodeID, hostname, and port is the correct approach. This ensures that the addressesIPs slice contains the complete addresses for each validator.
413-417: LGTM!
Writing the private key seed words to a file named key_seed.json in the clientDir is a good practice for secure storage of the mnemonic. The file name and location are appropriate.
421-421: LGTM!
Adding the new sdk.NewCoin with the cfg.DepositedTokens amount to the balances is correct. This ensures that the deposited tokens are included in the genesis balances for each validator.
Line range hint 431-439: LGTM!
The changes to set the custom app template, write the app configuration file, and read the configuration using ctx.Viper are all valid and follow the expected pattern. This ensures that the application configuration is properly set up for each validator.
469-478: LGTM!
The changes to sort the addressesIPs slice and set the persistentPeers field in the config.toml file for each validator are correct. This ensures that each validator has the correct persistent peers configuration based on the sorted addressesIPs.
Sorting the addressesIPs slice provides a deterministic order, and excluding the validator's own address from its persistentPeers list prevents self-connection attempts.
Writing the updated configuration to the config.toml file using tmcfg.WriteConfigFile is the appropriate way to persist the changes.
480-481: Verify the impact of the commission rate change.
The commissionRate is now set to 0.5 using sdkmath.LegacyNewDecFromStr. This value is passed to the initGenFiles function.
Verify that the commission rate change does not have any unintended consequences on the network's functionality and incentive structure. Ensure that the chosen commission rate aligns with the desired economic model of the network.
#!/bin/bash
# Description: Verify the impact of the commission rate change.
# Test 1: Check if the validators are created with the correct commission rate.
rg -A 10 -B 10 'stakingTypes\.Commission' | rg 'Rate: "0.5"'
# Test 2: Check if the network incentives and rewards are functioning as expected with the new commission rate.
rg -A 10 -B 10 'k\.hooks\.AfterValidatorCreated' | rg 'commission rate'371-376: Verify the initialization of node key and consensus key.
The genutil.InitializeNodeValidatorFiles function is used to initialize the node key and consensus private key files under the nodeDir. This is an important step in setting up the validator files.
Verify that the InitializeNodeValidatorFiles function is working as expected and the generated files are correctly placed in the config directory.
#!/bin/bash
# Description: Verify the initialization of node key and consensus key files.
# Test: Check if the node key and consensus key files are generated for each validator.
for i in $(seq 0 $((cfg.NumValidators-1))); do
nodeDir="$network.BaseDir/node$i/exocored"
ls "$nodeDir/config/node_key.json" "$nodeDir/config/priv_validator_key.json"
done117-120: Verify the impact of reducing the token amounts.
The DefaultConfig function is updated to initialize AccountTokens, DepositedTokens, and StakingTokens with smaller values using sdk.TokensFromConsensusPower. This change reduces the token amounts compared to the previous large values.
Ensure that reducing these token amounts does not adversely affect the functionality or testing of the network. Verify that the network can still operate correctly with these reduced values.
✅ Verification successful
Based on the gathered information, I can now provide a final response:
The reduced token amounts are safe and sufficient for testing purposes
The reduced token amounts (1000, 500, and 200 tokens) are still significantly higher than the minimum requirements found in the codebase:
- The
MinSelfDelegationparameter is set to 100 tokens - The network tests successfully initialize and run with these values
- No test assertions or validations fail due to insufficient token amounts
The changes maintain a proper ratio between account, deposit, and staking tokens while reducing the overall magnitude, which is a good practice for test environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the network operates correctly with reduced token amounts.
# Test 1: Check if the network can be started and validators created with the reduced token amounts.
rg -A 10 -B 10 'func New\(.*\*Network' | rg 'return network, nil'
# Test 2: Check if transactions can be successfully executed with the reduced token amounts.
rg -A 10 -B 10 'tx, err := val\.ClientCtx\.TxConfig\.TxEncoder\(\)\.Encode\(builder\.GetTx\(\)\)' | rg 'err == nil'
Length of output: 230
Script:
#!/bin/bash
# Let's verify the token amounts usage in tests and network setup
# Check where these token amounts are used in test scenarios
rg -A 5 -B 5 "TokensFromConsensusPower" testutil/
# Check for any test failures or assertions related to token amounts
rg -A 5 "require\.NoError|assert\.NoError" testutil/ | grep -i "token"
# Look for network initialization and validator setup
rg -A 10 "func New" testutil/network/
Length of output: 7963
Script:
#!/bin/bash
# Let's check test files to verify if these reduced amounts are sufficient
# Check test files that use this network setup
rg -A 10 "network.New\(" testutil/
# Check if there are any minimum token requirements or validations
rg -A 5 "MinSelfDelegation|MinDeposit" testutil/
# Look for any token-related test assertions or validations
rg -A 5 "require\.True|assert\.True" testutil/ | grep -i "token\|amount"
Length of output: 2252
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (8)
testutil/network/query.go (3)
12-35: Add method documentation and improve error handling.While the implementation is functional, consider these improvements:
- Add godoc comments for each method describing their purpose
- Add validation for
n.Validatorsto prevent panic if emptyExample implementation:
+// QueryOracle returns a query client for the oracle module using the first validator's context func (n *Network) QueryOracle() oracletypes.QueryClient { + if len(n.Validators) == 0 { + panic("no validators in network") + } return oracletypes.NewQueryClient(n.Validators[0].ClientCtx) }🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 18-18:
File is notgofumpt-ed (gofumpt)
16-19: Fix formatting in QueryBank method.Remove the extra newline between the return statement and the closing brace.
func (n *Network) QueryBank() banktypes.QueryClient { return banktypes.NewQueryClient(n.Validators[0].ClientCtx) - }🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 18-18:
File is notgofumpt-ed (gofumpt)
12-35: Consider supporting multiple validator queries.Currently, all query methods are hardcoded to use the first validator's context. Consider:
- Adding a parameter to specify which validator's context to use
- Or providing additional methods to query specific validators
Example enhancement:
// QueryOracleFromValidator returns a query client for the oracle module using the specified validator's context func (n *Network) QueryOracleFromValidator(validatorIndex int) oracletypes.QueryClient { if validatorIndex >= len(n.Validators) { panic(fmt.Sprintf("validator index %d out of range", validatorIndex)) } return oracletypes.NewQueryClient(n.Validators[validatorIndex].ClientCtx) }🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 18-18:
File is notgofumpt-ed (gofumpt)crypto/algo_ed25519.go (2)
4-4: Clean up commented importsThere are commented-out imports that should either be removed or uncommented if they'll be needed in the future. This helps maintain clean and clear code.
- // "github.com/cometbft/cometbft/crypto/ed25519" - // "github.com/evmos/evmos/v16/crypto/ethsecp256k1"Also applies to: 12-12
50-55: Enforce test-only usageSince this option is intended for testing only, consider adding runtime checks to prevent accidental usage in production.
func Ed25519Option() keyring.Option { + if !isTestEnvironment() { + panic("Ed25519Option is only for testing purposes") + } return func(options *keyring.Options) { options.SupportedAlgos = SupportedAlgorithms }testutil/network/util.go (1)
Line range hint
158-234: Consider modularizing genesis state initialization.The function handles initialization of multiple modules in a sequential manner. While functional, it could benefit from modularization for better maintainability.
Consider breaking down the initialization into module-specific helper functions:
+func initBasicModulesGenState(cfg Config) error { + // Initialize auth, bank, gov, crisis, evm modules +} + +func initValidatorModulesGenState(cfg Config, validators []*Validator, commissionRate sdkmath.LegacyDec) error { + // Initialize assets, operator, dogfood modules +} func initGenFiles(...) error { - // Current implementation + if err := initBasicModulesGenState(cfg); err != nil { + return err + } + return initValidatorModulesGenState(cfg, validators, commissionRate) }testutil/network/network.go (1)
485-486: Consider parameterizing commission rateThe commission rate is hardcoded to 0.5. Consider making it configurable through the Config struct to allow for different test scenarios.
type Config struct { // ... existing fields ... + CommissionRate string // the validator commission rate } func DefaultConfig() Config { // ... existing code ... + commissionRate: "0.5", } -commissionRate, _ := sdkmath.LegacyNewDecFromStr("0.5") +commissionRate, _ := sdkmath.LegacyNewDecFromStr(cfg.CommissionRate)testutil/network/tx.go (1)
14-14: Improve the function comment for clarityThe comment has grammatical errors and can be enhanced for better readability.
Suggested change:
-// GenSignedTx construct a tx with input messages and sign it privatekey stored in keyring +// GenSignedTx constructs a transaction with input messages and signs it with the private key stored in the keyring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
crypto/algo_ed25519.go(1 hunks)testutil/network/network.go(11 hunks)testutil/network/network_test.go(4 hunks)testutil/network/query.go(1 hunks)testutil/network/tx.go(1 hunks)testutil/network/util.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- testutil/network/network_test.go
🧰 Additional context used
🪛 GitHub Check: Run golangci-lint
crypto/algo_ed25519.go
[warning] 32-32:
unused-parameter: parameter 'path' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 44-44:
SA1019: ed25519.PrivKey is deprecated: PrivKey defines a ed25519 private key. NOTE: ed25519 keys must not be used in SDK apps except in a tendermint validator context. (staticcheck)
testutil/network/network.go
[failure] 19-19:
ST1019: package "cosmossdk.io/math" is being imported more than once (stylecheck)
[failure] 20-20:
ST1019(related information): other import of "cosmossdk.io/math" (stylecheck)
testutil/network/query.go
[failure] 18-18:
File is not gofumpt-ed (gofumpt)
testutil/network/tx.go
[failure] 29-29:
Error return value of txBuilder.SetMsgs is not checked (errcheck)
testutil/network/util.go
[failure] 17-17:
ST1019: package "github.com/cosmos/cosmos-sdk/types" is being imported more than once (stylecheck)
[failure] 18-18:
ST1019(related information): other import of "github.com/cosmos/cosmos-sdk/types" (stylecheck)
🔇 Additional comments (7)
testutil/network/query.go (1)
1-10: LGTM! Well-structured package and imports.
The package structure and imports are well-organized, with clear aliases for type packages.
crypto/algo_ed25519.go (1)
18-21: Verify supported algorithms configuration
The Ed25519 type is defined but not included in SupportedAlgorithms. If Ed25519 should be supported, consider adding it to the list.
testutil/network/util.go (1)
17-18:
Consolidate duplicate SDK type imports.
The same package github.com/cosmos/cosmos-sdk/types is imported twice with different aliases (sdk and sdktypes). This can lead to confusion and inconsistency in the codebase.
Apply this diff to consolidate the imports:
sdk "github.com/cosmos/cosmos-sdk/types"
-sdktypes "github.com/cosmos/cosmos-sdk/types"Update all references of sdktypes to sdk throughout the code.
Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 17-17:
ST1019: package "github.com/cosmos/cosmos-sdk/types" is being imported more than once (stylecheck)
[failure] 18-18:
ST1019(related information): other import of "github.com/cosmos/cosmos-sdk/types" (stylecheck)
testutil/network/network.go (4)
474-482: Well-structured peer configuration
Good improvement! Sorting the addressesIPs ensures deterministic peer connections across test runs, enhancing test stability and reproducibility.
114-114: Consider impact of reduced timeout
The TimeoutCommit has been reduced from 3s to 2s, and MinGasPrices increased to "10hua". While this might speed up tests, please verify that:
- 2s is sufficient for consensus in test scenarios
- The increased gas price doesn't impact existing tests
#!/bin/bash
# Search for timeout-related test failures
rg -A 5 "timeout.*exceeded"
# Search for gas price assertions in tests
rg -A 5 "MinGasPrices.*assert"Also applies to: 119-120
19-20:
Remove duplicate import of "cosmossdk.io/math"
Keep only the aliased import sdkmath "cosmossdk.io/math" and remove the unaliased one.
- "cosmossdk.io/math"
sdkmath "cosmossdk.io/math"Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 19-19:
ST1019: package "cosmossdk.io/math" is being imported more than once (stylecheck)
[failure] 20-20:
ST1019(related information): other import of "cosmossdk.io/math" (stylecheck)
82-84: Verify staking tokens comment accuracy
The comment for StakingTokens states it's "for every supported asset", but it's not clear how this is enforced or where the supported assets are defined. Please verify if this is accurate and consider adding more context.
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (30)
tests/e2e/oracle/oracle_test.go (2)
10-16: Add documentation for the test suite function.Consider adding a descriptive comment explaining the purpose and setup of this e2e test suite.
+// TestE2ESuite initializes and runs end-to-end tests for the oracle package +// using a single validator network configuration. func TestE2ESuite(t *testing.T) {
11-14: Consider extracting configuration values to constants.The network configuration values are hardcoded. Consider extracting these to named constants with explanatory comments, especially if these values will be reused across other test files.
+const ( + // DefaultValidatorCount defines the number of validators for e2e tests + DefaultValidatorCount = 1 +) + func TestE2ESuite(t *testing.T) { cfg := network.DefaultConfig() - cfg.NumValidators = 1 + cfg.NumValidators = DefaultValidatorCount cfg.CleanupDir = true cfg.EnableTMLogging = truetests/e2e/bank/bank_test.go (2)
10-16: Add documentation and consider test coverage improvements.While the test setup is functional, consider the following improvements:
- Add documentation explaining the purpose of this E2E test suite and its configuration choices
- Consider if single validator setup is sufficient for all E2E scenarios
- Document the expected behavior of
NewE2ETestSuiteAdd documentation like this:
+// TestE2ETestSuite initializes and runs the bank module's E2E test suite. +// It sets up a single validator network configuration with cleanup enabled +// and Tendermint logging disabled to focus on test outputs. func TestE2ETestSuite(t *testing.T) {
10-16: Expand test coverage for bank module operations.Since this PR is WIP and test cases are pending, consider adding the following test scenarios:
- Bank module query operations (balances, total supply)
- Transaction operations (send, multi-send)
- Error cases (insufficient funds, invalid addresses)
- IBC token transfers (if applicable)
Would you like me to help create a GitHub issue tracking these test scenarios or provide example test implementations?
tests/e2e/basesuite.go (2)
8-13: Add documentation for BaseSuite struct and its fields.Consider adding documentation to improve code maintainability:
+// BaseSuite provides a base test suite for E2E testing with network configuration and setup. type BaseSuite struct { suite.Suite + // Cfg holds the network configuration for the test suite Cfg network.Config + // Network represents the running test network instance Network *network.Network }
15-17: Add documentation and parameter validation to constructor.Consider these improvements for better maintainability and robustness:
+// NewBaseSuite creates a new BaseSuite instance with the provided network configuration. +// It initializes the Cfg field but leaves Network nil, which will be initialized in SetupSuite. func NewBaseSuite(cfg network.Config) *BaseSuite { + if cfg.NumValidators == 0 { + cfg.NumValidators = 1 // Set a default if not specified + } return &BaseSuite{Cfg: cfg} }tests/e2e/bank/suite.go (2)
8-13: Add documentation for the test suite struct and its fields.Consider adding GoDoc comments to improve code documentation:
+// E2ETestSuite defines a test suite for bank module E2E tests type E2ETestSuite struct { suite.Suite + // cfg holds the network configuration cfg network.Config + // network is the testing network instance network *network.Network }
15-17: Enhance constructor with documentation and validation.Consider adding documentation and basic validation:
+// NewE2ETestSuite creates a new E2E test suite with the given network configuration func NewE2ETestSuite(cfg network.Config) *E2ETestSuite { + if cfg.NumValidators == 0 { + cfg.NumValidators = 1 // Set default if not specified + } return &E2ETestSuite{cfg: cfg} }tests/e2e/oracle/suite.go (3)
8-13: Add documentation for the test suite struct.Consider adding a documentation comment for the
E2ETestSuitestruct to describe its purpose and usage. This helps other developers understand how to use and extend the test suite.+// E2ETestSuite defines a test suite for Oracle end-to-end testing. +// It provides network configuration and setup for testing Oracle functionality +// in a simulated network environment. type E2ETestSuite struct { suite.Suite cfg network.Config network *network.Network }
15-17: Add documentation for the constructor.The constructor should have a documentation comment explaining its purpose and parameters.
+// NewE2ETestSuite creates a new E2E test suite with the given network configuration. +// It returns an initialized but not yet set up test suite instance. func NewE2ETestSuite(cfg network.Config) *E2ETestSuite { return &E2ETestSuite{cfg: cfg} }
19-26: Improve SetupSuite implementation.Consider the following improvements to make the setup more robust and maintainable:
- Add cleanup in case of initialization failure
- Make the target block height configurable
- Add more descriptive error context
+// SetupSuite initializes the test network and ensures it reaches a minimum block height +// before proceeding with tests. func (s *E2ETestSuite) SetupSuite() { s.T().Log("setting up e2e test suite") var err error s.network, err = network.New(s.T(), s.T().TempDir(), s.cfg) - s.Require().NoError(err) + if err != nil { + s.T().Fatalf("failed to initialize test network: %v", err) + } + + // Ensure cleanup on initialization failure + defer func() { + if err != nil && s.network != nil { + s.network.Cleanup() + } + }() + + // Wait for network to produce blocks to ensure it's ready for testing + const minBlockHeight = 2 // Minimum height required for network stability _, err = s.network.WaitForHeight(2) - s.Require().NoError(err) + if err != nil { + s.T().Fatalf("failed waiting for network height: %v", err) + } }tests/e2e/oracle/tx.go (2)
10-10: Consider using standard time layout constants.Instead of defining a custom layout, consider using
time.RFC3339or other standard layouts from thetimepackage for better consistency and maintainability.-const layout = "2006-01-02 15:04:05" +// Use time.RFC3339 or other standard layouts as needed
33-39: Document or implement empty test functions.The empty test functions should either be implemented or documented with TODO comments explaining their intended purpose and test scenarios.
func (s *E2ETestSuite) TestCreatePriceNST() { - + // TODO: Implement test for NST price creation + // Test scenarios: + // 1. Create NST price with valid parameters + // 2. Verify price creation } func (s *E2ETestSuite) TestSlashing() { - + // TODO: Implement slashing test + // Test scenarios: + // 1. Trigger slashing conditions + // 2. Verify slashing penalties }🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 34-34:
File is notgofumpt-ed (gofumpt)
[failure] 38-38:
File is notgofumpt-ed (gofumpt)testutil/network/tx.go (2)
11-12: Improve function documentation.The current documentation is minimal. Consider adding details about:
- Parameter requirements
- Error conditions
- Why synchronous broadcast is used
- Example usage
-// SendTx construct and sign that tx with input msgs +// SendTx constructs and broadcasts a transaction with the given messages. +// It uses the first validator's client context and broadcasts synchronously. +// Parameters: +// - msgs: List of messages to include in the transaction (must not be empty) +// - keyName: Name of the key in the keyring to sign with +// - keyring: Keyring containing the signing key +// Returns an error if key lookup, address retrieval, or broadcast fails
25-34: Consider making gas adjustment configurable.The gas adjustment factor of 1.5 is hardcoded. Consider making this configurable through the network configuration.
+// In network/config.go: +type Config struct { + // ... existing fields ... + GasAdjustment float64 +} +// In this file: txf := tx.Factory{}. WithChainID(ctx.ChainID). WithKeybase(keyring). WithTxConfig(ctx.TxConfig). WithSignMode(signing.SignMode_SIGN_MODE_DIRECT). - WithGasAdjustment(1.5). + WithGasAdjustment(n.Config.GasAdjustment). WithAccountRetriever(ctx.AccountRetriever). WithGasPrices(n.Config.MinGasPrices). WithSimulateAndExecute(true)tests/e2e/util.go (1)
14-21: LGTM with a minor suggestion about context handling.The function correctly implements the balance query using the bank module. Consider accepting a context parameter instead of creating a background context internally, to allow better control over timeouts and cancellation from the caller's side.
-func QueryNativeCoinBalance(address sdk.AccAddress, n *network.Network) (*banktypes.QueryBalanceResponse, error) { - return n.QueryBank().Balance(context.Background(), &banktypes.QueryBalanceRequest{ +func QueryNativeCoinBalance(ctx context.Context, address sdk.AccAddress, n *network.Network) (*banktypes.QueryBalanceResponse, error) { + return n.QueryBank().Balance(ctx, &banktypes.QueryBalanceRequest{tests/e2e/bank/tx.go (5)
10-10: Add a descriptive comment for the test function.Consider adding a comment that describes the test scenario and its objectives for better documentation.
+// TestSendCoin validates the following scenarios: +// 1. A validator can send native coins to a new account +// 2. The new account can send coins to another new account func (s *E2ETestSuite) TestSendCoin() {
12-12: Fix typo in comment.The word "recieve" is misspelled and should be "receive".
- // generate a new account with ethsecp256k1 to recieve/send native coins (hua) + // generate a new account with ethsecp256k1 to receive/send native coins (hua)🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 12-12:
recieveis a misspelling ofreceive(misspell)
17-17: Consider extracting magic numbers into named constants.The amount
2000000should be defined as a constant at the package level for better maintainability and clarity.+const ( + // InitialTransferAmount represents the amount of coins to transfer in the first transaction + InitialTransferAmount = 2000000 +) - msg := banktypes.NewMsgSend(fromAddr, toAddr, sdk.NewCoins(sdk.NewCoin(s.network.Config.NativeDenom, sdkmath.NewInt(2000000)))) + msg := banktypes.NewMsgSend(fromAddr, toAddr, sdk.NewCoins(sdk.NewCoin(s.network.Config.NativeDenom, sdkmath.NewInt(InitialTransferAmount))))
34-37: Remove redundant comment and extract magic number.The comment on line 35 is redundant with the code. Also, consider extracting the transfer amount into a named constant.
+const ( + SecondaryTransferAmount = 100 +) msg = banktypes.NewMsgSend(toAddr, toAddr2, sdk.NewCoins(sdk.NewCoin(s.network.Config.NativeDenom, sdkmath.NewInt(100)))) - // send sendCoinMsg err = s.network.SendTx([]sdk.Msg{msg}, "user1", kr)
10-47: Consider enhancing test coverage and organization.The test function could be improved in the following ways:
- Break down the test into subtests using
t.Run()for better organization and clarity- Add negative test cases to verify proper error handling, such as:
- Insufficient balance
- Invalid addresses
- Zero amount transfers
This would provide more comprehensive test coverage of the bank module's functionality.
Would you like me to provide an example of how to restructure the test with these improvements?
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 12-12:
recieveis a misspelling ofreceive(misspell)crypto/algo_ed25519.go (3)
4-4: Clean up commented imports.Remove the commented imports if they are no longer needed. If they will be used later, add a TODO comment explaining the intention.
- // "github.com/cometbft/cometbft/crypto/ed25519" + // TODO: Switch to CometBFT's ed25519 implementation when X is implementedAlso applies to: 12-12
50-55: Enhance test safety measures.The function modifies global state through
SupportedAlgorithmswhich could affect other tests. Consider:
- Adding runtime checks to ensure this is only called in test environments
- Implementing a way to restore the previous algorithm list after tests
- Using a test-specific keyring configuration instead of modifying global state
Example implementation:
func Ed25519Option() keyring.Option { return func(options *keyring.Options) { if !isTestEnvironment() { panic("Ed25519Option is only for testing purposes") } // Store previous algorithms prevAlgos := options.SupportedAlgos options.SupportedAlgos = SupportedAlgorithms // Optional: Register cleanup in test context if t, ok := testing.T.(*testing.T); ok { t.Cleanup(func() { options.SupportedAlgos = prevAlgos }) } } }
1-55: Isolate test-only code to prevent production usage.This file contains several non-standard implementations and security bypasses that are marked as "mainly for testing". To prevent accidental usage in production:
- Move the file to a
testutilor similar test-specific directory- Rename the package to
cryptotestto make its purpose clear- Add build tags to prevent compilation in production builds:
//go:build testing // +build testingWould you like assistance in restructuring this code to better isolate test utilities?
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[warning] 32-32:
unused-parameter: parameter 'path' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 44-44:
SA1019: ed25519.PrivKey is deprecated: PrivKey defines a ed25519 private key. NOTE: ed25519 keys must not be used in SDK apps except in a tendermint validator context. (staticcheck)go.mod (1)
266-266: Remove TODO comment and provide clear documentation.The TODO comment in the replacement directive for evmos fork should be removed and replaced with a clear explanation of why this fork is necessary.
- // use exocore fork of evmos TODO + // use exocore fork of evmos for network testing compatibilitytestutil/network/tx_internal.go (4)
24-24: Correct the function comment for clarity and grammar.The function comment on line 24 contains grammatical errors that might reduce clarity. Please revise it for better readability.
Apply this diff to correct the function comment:
-// GenSignedTx construct a tx with input messages and sign it privatekey stored in keyring +// genSignedTxInternal constructs a transaction with input messages and signs it using the private key stored in the keyring.
29-29: Remove commented-out code to improve code cleanliness.The commented line
// record, err := ctx.Keyring.Key(keyName)seems unnecessary and could be removed to keep the code clean.Apply this diff:
- // record, err := ctx.Keyring.Key(keyName)
54-63: Use proper parsing forMinGasPricesinstead of substring manipulation.Subtracting the length of
n.Config.NativeDenomfromlen(n.Config.MinGasPrices)to extractminGasPriceStris error-prone and may not work correctly if the format changes or if multiple denominations are specified. Consider usingsdk.ParseDecCoinsto parseMinGasPricesand extract the minimum gas price for the desired denomination.Here's how you might refactor the code:
- l := len(n.Config.MinGasPrices) - len(n.Config.NativeDenom) - fee := sdkmath.ZeroInt() - if l > 0 { - minGasPriceStr := n.Config.MinGasPrices[:l] - minGasPrice, err := sdkmath.NewIntFromString(minGasPriceStr) - if err != nil { - return nil, err - } - fee = gasLimitInt.Mul(minGasPrice) - } + fee := sdkmath.ZeroInt() + minGasPrices, err := sdk.ParseDecCoins(n.Config.MinGasPrices) + if err != nil { + return nil, err + } + minGasPrice := minGasPrices.AmountOf(n.Config.NativeDenom) + feeDec := minGasPrice.MulInt64(int64(gasLimit)) + fee = feeDec.Ceil().TruncateInt()
73-73: Remove commented-out code to maintain code cleanliness.The commented line
// WithKeybase(ctx.Keyring)is unnecessary and can be removed to keep the code clean and maintainable.Apply this diff:
- // WithKeybase(ctx.Keyring).testutil/network/util.go (1)
244-244: Correct typo in commentThere's a typo in the comment. "statse" should be "state".
Apply this diff to fix the typo:
- // set oracle genesis statse + // set oracle genesis state
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
crypto/algo_ed25519.go(1 hunks)go.mod(1 hunks)tests/e2e/bank/bank_test.go(1 hunks)tests/e2e/bank/query.go(1 hunks)tests/e2e/bank/suite.go(1 hunks)tests/e2e/bank/tx.go(1 hunks)tests/e2e/basesuite.go(1 hunks)tests/e2e/oracle/oracle_test.go(1 hunks)tests/e2e/oracle/suite.go(1 hunks)tests/e2e/oracle/tx.go(1 hunks)tests/e2e/util.go(1 hunks)testutil/network/network.go(12 hunks)testutil/network/tx.go(1 hunks)testutil/network/tx_internal.go(1 hunks)testutil/network/util.go(5 hunks)
🧰 Additional context used
🪛 GitHub Check: Run golangci-lint
crypto/algo_ed25519.go
[warning] 32-32:
unused-parameter: parameter 'path' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 44-44:
SA1019: ed25519.PrivKey is deprecated: PrivKey defines a ed25519 private key. NOTE: ed25519 keys must not be used in SDK apps except in a tendermint validator context. (staticcheck)
tests/e2e/bank/tx.go
[failure] 12-12:
recieve is a misspelling of receive (misspell)
tests/e2e/oracle/tx.go
[failure] 14-14:
Error return value of s.network.WaitForHeight is not checked (errcheck)
[failure] 28-28:
Error return value of s.network.SendTx is not checked (errcheck)
[failure] 30-30:
Error return value of s.network.WaitForHeight is not checked (errcheck)
[failure] 34-34:
File is not gofumpt-ed (gofumpt)
[failure] 38-38:
File is not gofumpt-ed (gofumpt)
testutil/network/network.go
[failure] 19-19:
ST1019: package "cosmossdk.io/math" is being imported more than once (stylecheck)
🔇 Additional comments (26)
tests/e2e/oracle/oracle_test.go (3)
1-8: LGTM: Package and imports are well-structured.
The package name and imports are appropriate for an end-to-end test file.
15-15: 🛠️ Refactor suggestion
Add error handling for suite initialization.
The suite.Run() call assumes NewE2ETestSuite will always succeed. Consider adding error handling or validation for the suite initialization.
#!/bin/bash
# Check if NewE2ETestSuite has any error handling
rg -A 5 "NewE2ETestSuite.*error"10-16: Verify NewE2ETestSuite implementation.
The code calls NewE2ETestSuite() but this function is not defined in the current file.
✅ Verification successful
NewE2ETestSuite implementation is properly defined
The NewE2ETestSuite function is correctly implemented in tests/e2e/oracle/suite.go. It initializes and returns an E2ETestSuite struct with the provided network configuration. The implementation includes:
- A properly defined
E2ETestSuitestruct with required fields - A constructor function
NewE2ETestSuitethat takes anetwork.Configparameter - Associated setup methods for the test suite
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NewE2ETestSuite implementation
echo "Searching for NewE2ETestSuite implementation..."
rg -A 5 "func NewE2ETestSuite"
# Search for E2ETestSuite struct definition
echo "Searching for E2ETestSuite struct definition..."
ast-grep --pattern "type E2ETestSuite struct {
$$$
}"
Length of output: 512
Script:
#!/bin/bash
# Let's try a different approach to search for NewE2ETestSuite and E2ETestSuite
# Search in the same directory first
echo "Searching in oracle directory..."
rg -l "NewE2ETestSuite|E2ETestSuite" tests/e2e/oracle/
# Then search in the entire codebase
echo "Searching in entire codebase..."
rg -l "NewE2ETestSuite|E2ETestSuite"
# Get the content of files that contain these terms
echo "Content of relevant files:"
rg "type E2ETestSuite|func NewE2ETestSuite" -A 10
Length of output: 2396
tests/e2e/bank/bank_test.go (2)
1-8: LGTM! Package and imports are well-structured.
The package name matches the directory structure, and imports are appropriately organized.
10-16: Verify test coverage with multiple validators.
The current setup uses a single validator which might not catch issues that only appear in a multi-validator setup.
Let's check if there are other E2E tests that might require multiple validators:
tests/e2e/bank/query.go (2)
1-6: LGTM!
The package declaration and imports are appropriate and follow Go conventions.
9-9: Verify helper function implementation
The test relies on e2e.QueryNativeCoinBalance helper function. Let's ensure it properly handles edge cases and error conditions.
✅ Verification successful
Let me gather more information about the error handling in the bank query implementation.
Let me check the actual implementation of the bank query client to understand the error handling at the network level.
Based on the gathered information, I can now provide the final response:
Helper function implementation is adequately handling errors
The QueryNativeCoinBalance implementation in tests/e2e/util.go properly:
- Uses the network's bank query client which handles RPC communication
- Passes both address and denom validation through the standard SDK types
- Returns errors through the standard error interface
The function is simple and delegates the actual work to the well-tested SDK bank module, making additional error checks unnecessary in this helper.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of QueryNativeCoinBalance helper function
# Expected: Find the implementation and verify error handling
# Search for the QueryNativeCoinBalance function implementation
echo "Searching for QueryNativeCoinBalance implementation..."
ast-grep --pattern 'func QueryNativeCoinBalance($_, $_) $$ {
$$$
}'
# Look for error handling patterns in the implementation
echo "Checking error handling..."
rg -A 5 "QueryNativeCoinBalance.*\{"
Length of output: 707
Script:
#!/bin/bash
# Check the QueryBalance implementation in the bank module
echo "Checking bank.Balance implementation..."
rg -A 10 "Balance.*\{.*QueryBalanceRequest"
# Check if there are any test cases for error scenarios
echo "Checking test cases..."
rg -A 5 "TestQueryNativeCoinBalance|TestBalance.*func"
# Look for any related error handling patterns
echo "Checking error handling patterns..."
rg "ErrInvalidAddress|ErrInvalidDenom"
Length of output: 1229
Script:
#!/bin/bash
# Check the network bank query client implementation
echo "Checking network bank query implementation..."
rg -A 10 "type BankQuery.*struct|func.*BankQuery" tests/
# Check for any error handling in the e2e test setup
echo "Checking e2e test setup error handling..."
rg -A 5 "network.*Error|ValidateBasic" tests/e2e/
Length of output: 304
tests/e2e/basesuite.go (2)
1-7: LGTM!
The package name and imports are appropriate for the e2e testing framework.
19-26: 🛠️ Refactor suggestion
Enhance SetupSuite with better documentation and error handling.
While the basic setup is good, consider these improvements for better robustness:
+// SetupSuite implements suite.SetupAllSuite interface.
+// It initializes the test network and ensures it reaches a stable state before tests begin.
func (s *BaseSuite) SetupSuite() {
s.T().Log("setting up e2e test suite")
var err error
+
+ // Initialize network with provided configuration
s.Network, err = network.New(s.T(), s.T().TempDir(), s.Cfg)
s.Require().NoError(err)
+
+ // Ensure network is stable and producing blocks
+ s.T().Log("waiting for network to produce blocks")
_, err = s.Network.WaitForHeight(2)
s.Require().NoError(err)
+
+ s.T().Log("network setup completed successfully")
}Also consider adding:
- A configurable minimum height parameter
- A TearDownSuite method for proper cleanup
- Context with timeout for network initialization
tests/e2e/bank/suite.go (2)
1-7: LGTM! Package and imports are well-structured.
The package name and imports are appropriate for an E2E test suite.
1-26: Verify integration with other test components.
Since this is part of a larger test framework implementation:
- Ensure this test suite integrates with other components (gentx, queryclient, ed25519)
- Consider adding test coverage requirements
✅ Verification successful
Test suite integration is properly implemented
The verification shows that:
- Both bank and oracle test suites follow consistent patterns using the shared network configuration
- Common utility functions in
util.goare available across test suites - Test suites properly inherit from
BaseSuiteand implement consistentSetupSuitepatterns - Test coverage is managed through standard Go testing framework
No additional integration requirements or coverage configurations are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other test components and potential integration points
echo "Checking for related test components..."
rg -l "E2ETestSuite" tests/e2e/
echo "Checking for test coverage configuration..."
fd -g "*test*.go" tests/e2e/ --exec grep -l "coverage"
Length of output: 468
Script:
#!/bin/bash
# Check integration patterns between test components
echo "Checking integration patterns in bank suite..."
rg -A 5 "func.*Test" tests/e2e/bank/bank_test.go
echo "Checking oracle suite integration..."
rg -A 5 "func.*Test" tests/e2e/oracle/oracle_test.go
echo "Checking utility functions used across suites..."
cat tests/e2e/util.go
echo "Checking common setup patterns..."
rg "SetupSuite|TearDownSuite" tests/e2e/
Length of output: 2430
tests/e2e/oracle/suite.go (1)
3-7: LGTM: Clean and appropriate imports.
The imports are minimal and correctly include the necessary packages for E2E testing.
tests/e2e/oracle/tx.go (1)
1-8: LGTM: Package and imports are well-structured.
testutil/network/tx.go (2)
1-9: LGTM! Clean and well-organized imports.
The package declaration and imports are appropriate for the functionality being implemented.
12-37: Verify the impact on existing test cases.
This new SendTx method might be used by existing test cases. We should verify that the synchronous broadcast mode is appropriate for all use cases.
✅ Verification successful
Let's examine the usage of SendTx in these files to understand the context better.
Based on the search results, I can now generate the final response as the usage pattern is clear.
The synchronous broadcast mode is appropriate for the existing test cases
The current implementation using BroadcastSync mode is correct because:
- All test cases using
SendTxfollow it with eitherWaitForNextBlock()orWaitForHeight()calls - This pattern ensures proper transaction inclusion verification in the test scenarios
- Examples found in:
tests/e2e/bank/tx.go: UsesWaitForNextBlock()tests/e2e/oracle/tx.go: UsesWaitForHeight()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing test cases that might be affected by this new method
rg -l "SendTx.*\(" --type go
Length of output: 129
Script:
#!/bin/bash
# Check the usage of SendTx in test files
rg "SendTx.*\(" -A 5 --type go
Length of output: 1749
tests/e2e/util.go (1)
23-27: LGTM!
The function correctly implements coin creation using the native denomination from network configuration.
go.mod (2)
Line range hint 267-272: Security improvements in dependency replacements.
The replacement directives include important security fixes:
- Gin-gonic update addressing security advisory GHSA-h395-qcrw-5vmq
- Goleveldb fix for broken version
- Cosmos-SDK fork enabling Ledger functionality
These changes enhance the security posture of the codebase.
Line range hint 1-272: Verify dependency version compatibility for e2e testing.
The dependencies appear to be correctly configured for the e2e test network implementation. However, please verify the compatibility between the following critical dependencies:
github.com/evmos/cosmos-sdk v0.47.5-evmos.2github.com/ExocoreNetwork/evmos/v16 v16.0.3-0.20240828081344-d5cfcd34a812github.com/evmos/go-ethereum v1.10.26-evmos-rc2
These versions should be tested together to ensure they work correctly with the e2e test network components (gentx, queryclient, ed25519).
✅ Verification successful
Based on the search results, I can now generate a final response since I have enough information to verify the dependency compatibility:
Dependencies are correctly configured and compatible for e2e testing
The verification shows that:
- The Cosmos SDK fork (
github.com/evmos/cosmos-sdk v0.47.5-evmos.2) is consistently used across the codebase - The Evmos fork (
github.com/ExocoreNetwork/evmos/v16) and go-ethereum fork (github.com/evmos/go-ethereum) are properly integrated in key components:- EVM module and keeper
- Precompiles
- App initialization
- Transaction handling
- Network testing utilities
The dependencies work together to support e2e testing functionality, particularly for:
- Transaction signing and verification
- State transitions
- Network setup
- Contract interactions
- Keeper operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify dependency compatibility by checking import usage
# Test: Search for imports of these packages to ensure correct usage
echo "Checking cosmos-sdk imports..."
rg -l "github.com/cosmos/cosmos-sdk" --type go
echo "Checking evmos imports..."
rg -l "github.com/evmos/evmos/v16" --type go
echo "Checking go-ethereum imports..."
rg -l "github.com/ethereum/go-ethereum" --type go
Length of output: 22366
testutil/network/network.go (5)
19-20: Remove duplicate import.
This issue was previously identified in an earlier review. The package "cosmossdk.io/math" is imported twice.
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 19-19:
ST1019: package "cosmossdk.io/math" is being imported more than once (stylecheck)
489-497: LGTM! Improved deterministic network setup.
Good improvements:
- Sorting addressesIPs ensures consistent peer configuration
- Clear persistent peers setup for each validator
437-439: Simplification of validator token setup needs verification.
The token setup has been simplified to only use NativeDenom. Please verify:
- This doesn't break staking functionality
- Validators have sufficient tokens for operation
- Test scenarios aren't affected by removed token types
#!/bin/bash
# Check for dependencies on multiple token types
rg "sdk.NewCoin.*token"85-91: Verify the impact of reduced timeout and token amounts.
Several significant changes have been made:
- TimeoutCommit reduced from 3s to 2s
- Token amounts significantly reduced
- BondDenom replaced with NativeDenom
Please ensure these changes don't affect:
- Network stability with reduced timeout
- Test scenarios that depend on specific token amounts
Also applies to: 117-117, 124-126
✅ Verification successful
Let me check the actual usage of these configurations to better understand the impact.
Let me check one more thing to understand if there are any test failures or issues reported due to these changes.
Based on the gathered information, I can now provide the final response:
Changes to timeout and denominations are safe to proceed
The modifications are well-contained and aligned with the codebase:
- TimeoutCommit reduction to 2s is only used in test networks and doesn't affect production
- BondDenom to NativeDenom change is a rename that maintains consistency, as the value "hua" remains unchanged
- Token amount reductions are test-specific and don't impact production environments
The changes are properly reflected in:
- Test configurations in
testutil/network/network.go - Test setup files that reference these values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any test files that might be affected by these changes
rg -l "TimeoutCommit.*3.*Second"
rg -l "BondDenom"
Length of output: 302
Script:
#!/bin/bash
# Check the context of BondDenom usage and TimeoutCommit in the affected files
rg "BondDenom" -A 3 -B 3 testutil/network/network.go app/ante/utils/setup_test.go app/ante/cosmos/setup_test.go app/ante/evm/setup_test.go
rg "TimeoutCommit" -A 3 -B 3 testutil/network/network.go
Length of output: 4112
Script:
#!/bin/bash
# Check for any test files that might be using these configurations
rg "cfg\.TimeoutCommit" -A 3 -B 3
rg "DefaultConfig\(\)" -A 10 -B 3 testutil/network/network.go
Length of output: 1623
384-389: Verify the switch to Ed25519 signing algorithm.
The change from EthSecp256k1 to Ed25519 for key generation is significant. Please ensure:
- This aligns with the network's consensus requirements
- All dependent components support Ed25519
- No compatibility issues with existing keys
✅ Verification successful
Let me gather more information about the cryptographic setup to verify the changes.
Based on the extensive search results, I can now provide a final response regarding the switch to Ed25519.
The switch to Ed25519 signing algorithm is valid and properly supported.
The codebase shows:
- Ed25519 is explicitly supported as part of the supported algorithms (
crypto/algo_ed25519.go) - The consensus system is designed to work with Ed25519 keys (
types/keys/consensus.go) - The operator module has proper handling for Ed25519 consensus keys (
x/operator/types/tx.pb.go) - There's no tight coupling to EthSecp256k1 for consensus operations
The change aligns with the network's consensus requirements and maintains compatibility with the existing infrastructure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any dependencies on EthSecp256k1
rg "EthSecp256k1"
# Check for Ed25519 support in related files
rg "Ed25519"
Length of output: 1916
Script:
#!/bin/bash
# Check for supported algorithms configuration
ast-grep --pattern 'SupportedAlgorithms = $_'
# Check for any key generation or validation logic
ast-grep --pattern 'func NewMnemonic($$$) {
$$$
}'
# Check for any consensus key related configurations
rg -A 3 "consensus.*key" --type go
Length of output: 32896
testutil/network/util.go (3)
17-18: Consolidate duplicate imports of github.com/cosmos/cosmos-sdk/types
The imports at lines 17 and 18 both reference github.com/cosmos/cosmos-sdk/types but use different aliases (sdk and sdktypes). This can lead to confusion and inconsistencies in the codebase.
293-349: Avoid mutating global variables in NewGenStateAssets
The function NewGenStateAssets modifies the global variable DefaultGenStateAssets. Mutating global state can lead to unintended side effects and makes the code harder to test and maintain.
358-358: Add overflow protection for stakingAmount calculations
The multiplication of stakingAmount with the number of tokens could potentially overflow for large values.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (13)
tests/e2e/oracle/data.go (1)
9-13: Consider using time.Time for timestamp field.While the current implementation works, using
time.Timeinstead of string for theTimestampfield would provide better type safety and built-in parsing/formatting capabilities.type priceTime struct { Price string Decimal int32 - Timestamp string + Timestamp time.Time }tests/e2e/oracle/create-price.go (3)
14-47: Add documentation for test scenariosThe test involves complex interactions between validators and blockchain height progression. Consider adding documentation to explain the test flow and expected behavior.
Add comments explaining:
- The purpose of moving to specific heights
- Why two validators are needed
- The expected state after each operation
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 40-40: Useless assignment to local variable
This definition of err is never used.
49-55: Implement or document empty test functionsThe test functions
TestCreatePriceNSTandTestSlashingare empty. These should either be implemented or documented with TODO comments explaining their intended purpose.Would you like me to help create test cases for these functions or create GitHub issues to track their implementation?
57-67: Add documentation for utility functionsThe utility functions
moveToAndCheckandmoveNAndCheckwould benefit from documentation explaining their purpose and usage.Add godoc comments:
+ // moveToAndCheck advances the blockchain to a specific height and verifies the operation func (s *E2ETestSuite) moveToAndCheck(height int64) { + // moveNAndCheck advances the blockchain by n blocks and verifies each advancement func (s *E2ETestSuite) moveNAndCheck(n int64) {testutil/network/tx.go (3)
25-27: Fix typos in function name and commentsSeveral typos need to be corrected:
- Function name: "Createprice" should be "CreatePrice"
- Comments: "consturct" → "construct", "senario" → "scenario", "unexists" → "non-existent"
Apply this diff to fix the typos:
-// SendTxOracleCreatePrice consturct and sign that tx with input msgs, it's different from SendTx, since when we use ed25519 for oracle senario, we allowed that signer is an unexists account, this implementation skip the 'accoutn exists' related checks +// SendTxOracleCreatePrice constructs and signs a tx with input msgs. It's different from SendTx since when we use ed25519 for oracle scenario, +// we allow the signer to be a non-existent account. This implementation skips the 'account exists' related checks. // Also, if you want to sign some normal message (not oracle-create-price) with ed25519, just use SendTx is fine, we support ed25519 signing in keyring -func (n *Network) SendTxOracleCreateprice(msgs []sdk.Msg, keyName string, keyring keyring.Keyring) error { +func (n *Network) SendTxOracleCreatePrice(msgs []sdk.Msg, keyName string, keyring keyring.Keyring) error {
57-62: Improve error handling and documentationThe function returns the result of
PrintProtowithout documenting the expected output format or handling potential formatting errors.Consider returning the broadcast result directly and let the caller handle the formatting:
- return ctx.PrintProto(res) + return nilOr improve the documentation to explain the output format:
+// SendTxOracleCreatePrice constructs, signs and broadcasts a transaction. It returns an error if any operation fails. +// On success, it prints the transaction response in Protocol Buffer JSON format to the configured output writer. func (n *Network) SendTxOracleCreatePrice(msgs []sdk.Msg, keyName string, keyring keyring.Keyring) error {
64-89: Improve configuration flexibility and validationThe function has several areas for improvement:
- Broadcast mode is hardcoded to sync
- No validation for minGasPrice format
- No validation for gas adjustment value
- Factory pattern could be more fluent
Consider these improvements:
-func generateTxf(ctx client.Context, keyName string, kr keyring.Keyring, adjustment float64, minGasPrice string) (tx.Factory, client.Context, error) { +func generateTxf(ctx client.Context, keyName string, kr keyring.Keyring, opts TxFactoryOptions) (tx.Factory, client.Context, error) { + if opts.GasAdjustment <= 0 { + return txf, ctx, fmt.Errorf("gas adjustment must be positive") + } + if opts.MinGasPrice == "" { + return txf, ctx, fmt.Errorf("min gas price cannot be empty") + } // ... existing code ... - txf = tx.Factory{}. - WithChainID(ctx.ChainID). + txf = tx.NewFactory(ctx.ChainID). WithKeybase(kr). WithTxConfig(ctx.TxConfig). WithSignMode(signing.SignMode_SIGN_MODE_DIRECT). - WithGasAdjustment(adjustment). + WithGasAdjustment(opts.GasAdjustment). WithAccountRetriever(ctx.AccountRetriever). - WithGasPrices(minGasPrice). + WithGasPrices(opts.MinGasPrice). - WithSimulateAndExecute(true) + WithSimulateAndExecute(opts.SimulateAndExecute) - ctx.BroadcastMode = flags.BroadcastSync + ctx.BroadcastMode = opts.BroadcastMode return txf, ctx, nil } +// TxFactoryOptions contains configurable options for transaction factory +type TxFactoryOptions struct { + GasAdjustment float64 + MinGasPrice string + BroadcastMode flags.BroadcastMode + SimulateAndExecute bool +}go.mod (1)
269-270: Version pinning recommendation for evmos fork.The evmos fork is pinned to a specific version tag. Consider adding a comment explaining why this specific version is required.
Add a comment explaining the version requirement:
// use Evmos geth fork + // Pin to rc2 for compatibility with evmos v16.0.0 github.com/ethereum/go-ethereum => github.com/evmos/go-ethereum v1.10.26-evmos-rc2app/ante/cosmos/sigverify.go (3)
73-75: Enhance error message for better debuggingWhile the signature verification logic is correct, the error message could be more descriptive to aid in debugging.
- return ctx, sdkerrors.ErrInvalidPubKey.Wrapf("pubKey does not match signer address %s with signer index: %d", signers[i], i) + return ctx, sdkerrors.ErrInvalidPubKey.Wrapf("public key verification failed: expected signer %s but got %s at index %d", signers[i], pk.Address(), i)
Line range hint
1-577: Review architectural approach for oracle transaction validationThe current implementation creates a separate validation path for oracle transactions by bypassing standard Cosmos SDK transaction validation mechanisms. This architectural decision has several implications:
- It breaks the uniform transaction processing model of Cosmos SDK.
- It makes security auditing more complex due to special cases.
- It might make future upgrades and maintenance more difficult.
Consider implementing oracle transaction validation using Cosmos SDK's standard validation pipeline with custom validation decorators instead of bypassing the existing ones.
Line range hint
1-577: Standardize error handling across decoratorsThe error handling patterns vary across different decorators. Consider standardizing the approach:
- Use consistent error wrapping patterns.
- Provide detailed context in error messages.
- Consider adding error codes for different validation failures.
Example improvement:
- return ctx, sdkerrors.ErrTxDecode.Wrap("invalid transaction type") + return ctx, sdkerrors.ErrTxDecode.Wrapf( + "invalid transaction type: expected SigVerifiableTx, got %T", + tx, + )testutil/network/network.go (2)
488-496: Well-structured peer configuration implementationThe new code segment efficiently handles peer configuration:
- Sorts address IPs for consistent peer ordering
- Sets persistent peers for each validator
- Properly writes configuration to files
However, consider adding error handling for the config file write operation.
Consider adding error handling:
-tmcfg.WriteConfigFile(filepath.Join(cfgTmp.RootDir, "config", "config.toml"), cfgTmp) +err := tmcfg.WriteConfigFile(filepath.Join(cfgTmp.RootDir, "config", "config.toml"), cfgTmp) +if err != nil { + return nil, fmt.Errorf("failed to write validator %d config file: %w", i, err) +}
499-500: Consider making commission rate configurableThe commission rate is hardcoded to "0.5". Consider making this a configurable parameter in the Config struct for better flexibility in testing different scenarios.
Add a new field to Config struct:
type Config struct { // ... existing fields ... + CommissionRate string // default: "0.5" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
app/ante/cosmos/sigverify.go(1 hunks)crypto/algo_ed25519.go(1 hunks)go.mod(1 hunks)tests/e2e/oracle/create-price.go(1 hunks)tests/e2e/oracle/data.go(1 hunks)tests/e2e/oracle/oracle_test.go(1 hunks)testutil/network/network.go(12 hunks)testutil/network/tx.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/oracle/oracle_test.go
🧰 Additional context used
📓 Learnings (1)
testutil/network/network.go (1)
Learnt from: bwhour
PR: ExocoreNetwork/exocore#222
File: precompiles/avs/avs_test.go:8-11
Timestamp: 2024-11-10T20:28:27.115Z
Learning: `math/big` and `cosmossdk.io/math` are different libraries and should not be considered duplicate imports.
🪛 GitHub Check: CodeQL
tests/e2e/oracle/create-price.go
[warning] 40-40: Useless assignment to local variable
This definition of err is never used.
tests/e2e/oracle/data.go
[warning] 33-33: Calling the system time
Calling the system time may be a possible source of non-determinism
[warning] 42-42: Calling the system time
Calling the system time may be a possible source of non-determinism
🔇 Additional comments (13)
tests/e2e/oracle/data.go (2)
1-7: LGTM! Clean package structure and imports.
The package is well-organized with appropriate imports.
15-30: LGTM! Well-structured conversion methods.
The methods provide clean conversion between internal and external types.
crypto/algo_ed25519.go (2)
19-23: LGTM!
The struct definition and Name method implementation are correct and straightforward.
36-43: Duplicate of previous review comment
The security concerns about validation and deprecated type usage have been previously raised.
tests/e2e/oracle/create-price.go (2)
1-10: LGTM: Package structure and imports are well organized.
1-67: Verify test suite initialization
The test file uses E2ETestSuite, but the suite setup is not visible in this file.
Let's verify the test suite setup:
✅ Verification successful
Let me gather more information about the test suite setup and initialization:
Test suite is properly initialized
The E2ETestSuite is well-structured with proper initialization:
- Defined in
tests/e2e/oracle/suite.gowith required fields (network config and network instance) - Implements
SetupSuite()that initializes the network and waits for initial blocks - The test methods in the reviewed file use the initialized network instance for testing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for E2ETestSuite setup in related files
echo "Searching for test suite setup..."
rg -A 10 "type E2ETestSuite struct" tests/e2e/
Length of output: 1050
Script:
#!/bin/bash
# Search for SetupSuite or similar setup methods in the oracle test suite
echo "Searching for test suite setup methods..."
rg -A 10 "func \(.*E2ETestSuite\) Setup" tests/e2e/oracle/
Length of output: 606
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 40-40: Useless assignment to local variable
This definition of err is never used.
go.mod (3)
Line range hint 271-272: Verify security fix for gin-gonic/gin.
The replacement addresses security advisory GHSA-h395-qcrw-5vmq. Let's verify if v1.9.1 is still the latest secure version.
#!/bin/bash
# Description: Check for newer versions of gin-gonic/gin and any new security advisories
# Check latest version
gh api repos/gin-gonic/gin/releases/latest --jq .tag_name
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/gin-gonic/gin") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'268-269: Verify gogoproto version compatibility.
The replace directive for gogoproto is marked with "fix cosmos-sdk error". Let's verify if this version is still required.
#!/bin/bash
# Description: Check gogoproto version compatibility with cosmos-sdk
# Search for gogoproto version requirements in cosmos-sdk
cd /Users/linqing/workplace/github.com/leonz/evmos-cosmossdk/cosmos-sdk
rg -A 5 "github.com/cosmos/gogoproto" go.mod
# Check for any version-related issues
gh issue list -R cosmos/cosmos-sdk -l "dependencies,gogoproto" --json number,title,state | jq -r '.[] | "\(.number): \(.title) [\(.state)]"'265-267: Verify the commented-out replace directive.
The commented-out replace directive suggests a transition from evmos/cosmos-sdk to a local fork. Please verify if this change is intentional and if the local fork contains necessary changes.
app/ante/cosmos/sigverify.go (1)
Line range hint 273-290: Review oracle transaction signature verification
The current implementation has several security considerations:
- The TODO comment about multi-sig support needs to be addressed.
- The signature verification for oracle transactions skips some standard checks.
- There's no explicit error handling if
VerifySignaturefails.
Let's verify the oracle transaction handling across the codebase:
testutil/network/network.go (3)
19-20: Skip review of duplicate imports
A past review comment already addresses the duplicate import of "cosmossdk.io/math". The comment is still valid and applicable.
85-101: Config struct changes improve clarity and functionality
The changes to the Config struct are well-structured:
- Removed
BondDenomin favor ofNativeDenomfor better semantic clarity - Added
DepositedTokensfor managing staked assets - Updated comments to better describe field purposes
379-406: Improved key generation and node initialization process
The changes enhance the node setup process:
- Uses Ed25519 for validator consensus keys
- Properly initializes node validator files from mnemonic
- Generates and stores node IDs and addresses systematically
tests/e2e/oracle/create-price.go
Outdated
| priceTest1 := price1.updateTimestamp() | ||
| priceTimeDetID := priceTest1.getPriceTimeDetID("9") |
There was a problem hiding this comment.
Fix undefined variable reference
The variable price1 is undefined. This will cause compilation errors.
Please define the price1 variable or use the correct reference. Consider adding a helper function to create test price data:
func createTestPrice() *oracletypes.Price {
return &oracletypes.Price{
// Add necessary fields
}
}
| if err != nil { | ||
| return err | ||
| } | ||
| gasAdjusted := uint64(txf.GasAdjustment() * float64(simRes.GasInfo.GasUsed)) |
Check notice
Code scanning / CodeQL
Floating point arithmetic Note test
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (15)
x/oracle/keeper/aggregator/util.go (1)
5-10: Add documentation for the copyBigInt functionThe implementation is correct and safely handles nil cases. However, consider adding a documentation comment to explain its purpose and behavior, especially since it's used across multiple places in the aggregator package.
+// copyBigInt creates a deep copy of a big.Int pointer. +// Returns nil if the input is nil, otherwise returns a new independent copy. func copyBigInt(i *big.Int) *big.Int {tests/e2e/oracle/oracle_test.go (2)
10-16: Add documentation for the test suite setupThe test function would benefit from documentation explaining the purpose of the e2e test suite and why specific configuration values were chosen (e.g., why 4 validators).
Consider adding a doc comment like:
+// TestE2ESuite initializes and runs the end-to-end test suite for the oracle package. +// It configures a network with 4 validators to ensure proper consensus and price +// feed validation across multiple nodes. func TestE2ESuite(t *testing.T) {
11-14: Consider extracting configuration values as constantsThe network configuration values (4 validators, cleanup settings) could be extracted as named constants to improve maintainability and make the configuration more explicit.
Consider refactoring like this:
+const ( + // DefaultValidatorCount defines the number of validators required for oracle consensus + DefaultValidatorCount = 4 +) + func TestE2ESuite(t *testing.T) { cfg := network.DefaultConfig() - cfg.NumValidators = 4 + cfg.NumValidators = DefaultValidatorCount cfg.CleanupDir = true cfg.EnableTMLogging = truetests/e2e/oracle/data.go (2)
9-13: Consider improving type safety and adding documentation.The struct could benefit from:
- Using a more appropriate type for Price (e.g., decimal.Decimal)
- Adding field validation
- Including documentation for the struct and its fields
+// priceTime represents a price entry with its metadata for testing purposes type priceTime struct { + // Price represents the asset price as a string (consider using decimal.Decimal) Price string + // Decimal indicates the number of decimal places in the price Decimal int32 + // Timestamp represents the UTC time when the price was recorded Timestamp string }
49-60: Document test data significance.The test price values (199999 and 299999) seem arbitrary. Consider adding comments explaining what these values represent and why they were chosen for testing.
var ( + // price1 represents a test price of 1.99999 with 18 decimal places price1 = priceTime{ Price: "199999", Decimal: 18, Timestamp: time.Now().UTC().Format(layout), } + // price2 represents a test price of 2.99999 with 18 decimal places price2 = priceTime{ Price: "299999", Decimal: 18, Timestamp: time.Now().UTC().Format(layout), } )x/oracle/keeper/aggregator/calculator.go (1)
Line range hint
1-190: Consider architectural improvements for scalability.The current implementation maintains multiple price lists and creates deep copies, which could impact performance at scale. Consider the following improvements:
- Cache confirmed prices to avoid redundant calculations
- Implement pruning for old rounds to manage memory usage
- Consider using a more efficient data structure for price lookups
Would you like me to provide a detailed proposal for implementing these improvements?
x/oracle/keeper/aggregator/aggregator.go (1)
Line range hint
1-200: Address architectural concerns before production use.Several critical issues need attention:
Round ID Comparison: Using string comparison (
id < priceSourceRound.detID) for round IDs is unsafe and could lead to incorrect ordering. Consider using numeric types.Error Handling: Price parsing with
SetStringignores errors:pTR.price, _ = (&big.Int{}).SetString(pSource.Prices[0].Price, 10)
Incomplete Features: Multiple TODOs indicate unfinished functionality:
- Different consensus modes
- Rule implementations
- Proper error handling
Mock Implementation: The consensus mechanism is marked as "mock way to suite V1" which needs proper implementation.
Consider these fixes:
- if id := agg.dsPrices[priceSourceRound.sourceID]; len(id) == 0 || (len(id) > 0 && id < priceSourceRound.detID) { + roundID, err := strconv.ParseUint(priceSourceRound.detID, 10, 64) + if err != nil { + return fmt.Errorf("invalid round ID: %v", err) + } + currentRound, err := strconv.ParseUint(id, 10, 64) + if err != nil && len(id) > 0 { + return fmt.Errorf("invalid current round: %v", err) + } + if len(id) == 0 || currentRound < roundID {- pTR.price, _ = (&big.Int{}).SetString(pSource.Prices[0].Price, 10) + var ok bool + pTR.price, ok = (&big.Int{}).SetString(pSource.Prices[0].Price, 10) + if !ok { + return fmt.Errorf("invalid price format: %s", pSource.Prices[0].Price) + }Before deploying to production:
- Implement proper consensus mechanisms
- Add comprehensive error handling
- Complete the TODO items
- Add thorough testing for edge cases
Would you like me to help create GitHub issues to track these architectural improvements?
tests/e2e/oracle/create_price.go (5)
15-24: Enhance test case documentation readabilityConsider improving the documentation:
- Complete the empty "nonce" section or remove it if not needed
- Format test cases using a more structured approach, e.g.:
/* Test Cases (requiring >2/3 validator power): Case 1: Initial Price Submission - Block 1.1: V1 submits price P1 → No round 1 price (insufficient power) - Block 1.2: V2 & V3 submit price P1 → Round 1 price P1 confirmed (sufficient power) Case 2: Failed Price Update ... */
36-37: Remove commented-out codeRemove the commented-out code for validator 3 if it's not needed. If it will be used in future tests, add a TODO comment explaining the intention.
49-49: Define block height constantsConsider defining constants for block heights instead of using magic numbers (10, 11, 20, 30, 40). This would make the test more maintainable and the progression of blocks more clear.
+ const ( + case1StartBlock = 10 + case2StartBlock = 20 + case3StartBlock = 30 + case4StartBlock = 40 + )Also applies to: 60-60, 90-90, 118-118, 147-147
26-164: Refactor repeated test patternsConsider creating helper functions for common operations to reduce code duplication:
- Price message creation
- Validator transaction submission
- Price verification
Example helper function:
func (s *E2ETestSuite) submitValidatorPrice( creator sdk.AccAddress, kr keyring.Keyring, valKey string, priceSource *oracletypes.PriceSource, height int64, ) error { msg := oracletypes.NewMsgCreatePrice(creator.String(), 1, []*oracletypes.PriceSource{priceSource}, height, 1) return s.network.SendTxOracleCreateprice([]sdk.Msg{msg}, valKey, kr) }🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 163-163:
File is notgofumpt-ed (gofumpt)
166-172: Document empty test functionsAdd TODO comments to empty test functions explaining their intended purpose and implementation timeline.
func (s *E2ETestSuite) TestCreatePriceNST() { + // TODO: Implement negative scenario tests for price creation + // - Invalid price submissions + // - Incorrect validator signatures + // - Out of order submissions } func (s *E2ETestSuite) TestSlashing() { + // TODO: Implement slashing tests for malicious behavior + // - Double signing + // - Invalid price submissions + // - Timeout violations }🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 167-167:
File is notgofumpt-ed (gofumpt)go.mod (1)
266-266: Remove TODO comment or provide contextThe TODO comment in the replace directive for evmos fork needs to be addressed or removed. If there are pending tasks related to this fork, they should be documented properly.
Apply this change:
- // use exocore fork of evmos TODO + // use exocore fork of evmos for custom modificationstestutil/network/network.go (2)
85-92: Review the Config struct field changes.The changes look good, but please add documentation for the new
DepositedTokensfield to clarify its purpose and relationship with other token fields.AccountTokens math.Int // the amount of system tokens(e.g. 1000hua) - DepositedTokens math.Int + DepositedTokens math.Int // the amount of tokens deposited for staking/validation StakingTokens math.Int // the amount of tokens each validator stakes for every supporte asset
436-438: Update token denomination handling.The commented-out code suggests a transition in token denomination handling. Please clean up the old commented code since it's no longer needed.
balances := sdk.NewCoins( - // sdk.NewCoin(fmt.Sprintf("%stoken", nodeDirName), cfg.AccountTokens), - // sdk.NewCoin(cfg.BondDenom, cfg.DepositedTokens), sdk.NewCoin(cfg.NativeDenom, cfg.AccountTokens), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
go.mod(1 hunks)tests/e2e/oracle/create_price.go(1 hunks)tests/e2e/oracle/data.go(1 hunks)tests/e2e/oracle/oracle_test.go(1 hunks)testutil/network/network.go(12 hunks)x/oracle/keeper/aggregator/aggregator.go(2 hunks)x/oracle/keeper/aggregator/calculator.go(1 hunks)x/oracle/keeper/aggregator/util.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
testutil/network/network.go (1)
Learnt from: bwhour
PR: ExocoreNetwork/exocore#222
File: precompiles/avs/avs_test.go:8-11
Timestamp: 2024-11-12T10:03:10.791Z
Learning: `math/big` and `cosmossdk.io/math` are different libraries and should not be considered duplicate imports.
🪛 GitHub Check: Run golangci-lint
tests/e2e/oracle/create_price.go
[failure] 163-163:
File is not gofumpt-ed (gofumpt)
[failure] 167-167:
File is not gofumpt-ed (gofumpt)
[failure] 179-179:
func (*E2ETestSuite).moveNAndCheck is unused (unused)
tests/e2e/oracle/data.go
[warning] 38-38:
unused-parameter: parameter 'sourceID' seems to be unused, consider removing or renaming it as _ (revive)
testutil/network/network.go
[failure] 19-19:
ST1019: package "cosmossdk.io/math" is being imported more than once (stylecheck)
[failure] 20-20:
ST1019(related information): other import of "cosmossdk.io/math" (stylecheck)
🔇 Additional comments (12)
x/oracle/keeper/aggregator/util.go (1)
5-10: Add unit tests for copyBigInt
As mentioned in the PR objectives about adding test cases, please ensure this utility function is covered by unit tests. The tests should verify:
- Copying non-nil big.Int values
- Handling nil input
- Verifying the returned copy is independent (modifications don't affect the original)
Would you like me to help generate comprehensive unit tests for this function?
tests/e2e/oracle/oracle_test.go (1)
1-9: LGTM: Package and imports are well-structured
The package name and imports are appropriate for an end-to-end test file, providing the necessary testing utilities and suite functionality.
x/oracle/keeper/aggregator/calculator.go (1)
78-86: LGTM! Good improvement in big.Int handling.
The use of copyBigInt instead of direct assignments improves safety by preventing unintended mutations through shared references. This is particularly important for consensus-critical values like price and power.
Let's verify the copyBigInt implementation is available and properly used:
Consider adding the following safety measures:
- Add debug logging when copies are made to help trace any potential issues
- Consider implementing value range validation for price and power values
✅ Verification successful
Verified: copyBigInt is correctly implemented and appropriately used across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify copyBigInt implementation and usage
# Expected: Find copyBigInt definition and all its usages
# Find copyBigInt definition
echo "=== copyBigInt definition ==="
ast-grep --pattern 'func copyBigInt($_) *big.Int { $$$}'
# Find all copyBigInt usages
echo -e "\n=== copyBigInt usages ==="
rg 'copyBigInt\(' -A 2
Length of output: 2047
x/oracle/keeper/aggregator/aggregator.go (1)
53-55: Verify the implementation of copyBigInt and add documentation.
The changes consistently use copyBigInt for deep copying big.Int values, which is good for preventing shared references. However:
- Let's verify the implementation of
copyBigInt - The deep copy requirement should be documented
Add documentation to explain why deep copying is necessary:
func (agg *aggregator) copy4CheckTx() *aggregator {
+ // Creates a deep copy of the aggregator for CheckTx validation
+ // Deep copying is required to prevent shared state between CheckTx and DeliverTx
ret := &aggregator{Also applies to: 65-66, 71-71
✅ Verification successful
copyBigInt implementation is correct. Add documentation for clarity.
- Document the purpose and necessity of deep copying using
copyBigIntinx/oracle/keeper/aggregator/util.go.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of copyBigInt function
# Expected: Should find the implementation in util.go or similar files
# Search for copyBigInt implementation
rg -p "func copyBigInt" x/oracle/keeper/
# Search for imports of the file containing copyBigInt
rg -p "\"math/big\"" x/oracle/keeper/
Length of output: 898
Script:
# Retrieve the implementation of copyBigInt function in x/oracle/keeper/aggregator/util.go
ast-grep --pattern 'func copyBigInt($_)' x/oracle/keeper/aggregator/util.go
Length of output: 401
tests/e2e/oracle/create_price.go (1)
73-75: Add price value validation
The test verifies the presence of prices but doesn't validate the actual price values. Consider adding assertions to verify the correctness of the price values themselves.
Also applies to: 99-102, 132-135, 159-162
go.mod (3)
Line range hint 270-271: Security fix properly implemented
The replacement of gin-gonic/gin to address security advisory GHSA-h395-qcrw-5vmq is correctly implemented. The comment properly documents the security consideration.
267-267: Verify cosmos-sdk compatibility
The gogoproto version fix for cosmos-sdk error needs verification to ensure compatibility across the cosmos ecosystem packages.
Run the following script to verify version compatibility:
✅ Verification successful
Cosmos-SDK Compatibility Verified
The cosmos-sdk version v0.47.5-evmos.2 and gogoproto version v1.4.10 are compatible across the cosmos ecosystem packages. No version conflicts were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check version compatibility between cosmos-related packages
# Check cosmos-sdk version in replace directive
echo "Checking cosmos-sdk version..."
rg "github.com/cosmos/cosmos-sdk =>" -A 1
# Check gogoproto version in replace directive
echo "Checking gogoproto version..."
rg "github.com/cosmos/gogoproto =>" -A 1
# Check for potential version conflicts in cosmos packages
echo "Checking for version conflicts..."
rg "cosmossdk.io/" --type go
Length of output: 17112
Line range hint 4-4: Verify Go version compatibility
The Go version 1.21.12 is quite specific. Ensure this version requirement aligns with all dependencies, especially the cosmos ecosystem packages.
Run the following script to check Go version compatibility:
✅ Verification successful
Go version compatibility verified successfully
All dependencies align with Go version 1.21.12, and no conflicting build constraints were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Go version compatibility with dependencies
# Check for any go.mod files in dependencies that might specify different Go versions
echo "Checking Go versions in dependencies..."
fd -t f "go.mod$" -x rg "^go \d+\.\d+(\.\d+)?"
# Check for any build constraints in .go files
echo "Checking Go build constraints..."
rg "^//\s*\+build\s+go\d+\.\d+(\.\d+)?"
Length of output: 265
Script:
#!/bin/bash
# Description: Comprehensive check for Go version compatibility across all dependencies
echo "Listing all go.mod files and their Go versions..."
fd -t f "go.mod$" | xargs -I {} rg "^go \d+\.\d+(\.\d+)?" {} --files-with-matches --no-filename --only-matching
echo "Searching for Go build constraints in .go files..."
rg "^//\s*go:\s*build\s+go\d+\.\d+(\.\d+)?" --type go
Length of output: 400
testutil/network/network.go (4)
488-496: LGTM: Improved peer configuration.
Good improvement in peer configuration:
- Sorting addressesIPs ensures consistent peer configuration
- Each validator gets all other validators as persistent peers
- Configuration is properly persisted to config.toml
19-20:
Remove duplicate import of cosmossdk.io/math.
The package is imported twice, once as a direct import and once with an alias. Keep only the aliased import.
- "cosmossdk.io/math"
sdkmath "cosmossdk.io/math"Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 19-19:
ST1019: package "cosmossdk.io/math" is being imported more than once (stylecheck)
[failure] 20-20:
ST1019(related information): other import of "cosmossdk.io/math" (stylecheck)
379-389: Review the switch from eth-secp256k1 to ed25519.
The change from eth-secp256k1 to ed25519 for key generation is a significant architectural change. Please ensure:
- All dependent systems support ed25519 keys
- This change is documented in the changelog
- Migration path is provided for existing test keys
124-126: Verify token amounts are sufficient for test scenarios.
The token amounts have been significantly reduced:
- AccountTokens: 1000 power
- DepositedTokens: 500 power
- StakingTokens: 200 power
Please ensure these amounts are sufficient for your test scenarios, especially with 4 validators.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (16)
crypto/algo_ed25519.go (2)
28-29: Improve documentation for non-standard key derivationThe comments mention compatibility with
genutilpackage but lack crucial details:
- What specific compatibility issues are being addressed?
- Why is standard SLIP-0010 derivation not suitable?
- What are the security implications of this approach?
Please enhance the documentation to explain these aspects for better maintainability.
Also applies to: 32-32
50-50: Clarify usage restrictions in documentationThe comment "mainly used for test" is ambiguous. If this is strictly for testing purposes, the documentation should explicitly state:
- This should never be used in production
- What specific test scenarios this is intended for
- What alternatives should be used in production
Consider updating the comment to be more explicit about these restrictions.
testutil/network/genesis_data.go (2)
42-51: Remove or document commented code alternativesThere are commented alternatives for initializing
DefaultGenStateOperatorandDefaultGenStateDelegation. Either:
- Remove the commented code if it's no longer needed
- Document why we're using empty states instead of DefaultGenesis()
- Use the DefaultGenesis() approach consistently across all states
60-70: Add function documentationAdd a documentation comment for the
NewTestTokenfunction explaining its purpose and parameters:+// NewTestToken creates a new StakingAssetInfo for testing purposes. +// Parameters: +// name: The token name +// metaInfo: Additional information about the token +// address: The token's contract address +// chainID: The LayerZero chain ID +// amount: The total staking amount func NewTestToken(name, metaInfo, address string, chainID uint64, amount int64) assetstypes.StakingAssetInfo {testutil/network/network.go (5)
84-91: Review field naming and documentation.The Config struct changes look good, but consider:
- The comment for
AccountTokenscould be more specific about its purpose- The
StakingTokenscomment mentions "supporte" which has a typo- AccountTokens sdkmath.Int // the amount of system tokens(e.g. 1000hua) + AccountTokens sdkmath.Int // the amount of tokens initially allocated to each account (e.g. 1000hua) - StakingTokens sdkmath.Int // the amount of tokens each validator stakes for every supporte asset + StakingTokens sdkmath.Int // the amount of tokens each validator stakes for every supported asset
383-388: Consider error handling improvement.The error handling for keyring algorithms could be more robust. Consider combining the error checks.
- keyringAlgos, _ := kb.SupportedAlgorithms() - algo, err := keyring.NewSigningAlgoFromString(string(cosmoshd.Ed25519Type), keyringAlgos) - if err != nil { - return nil, err - } + keyringAlgos, err := kb.SupportedAlgorithms() + if err != nil { + return nil, fmt.Errorf("failed to get supported algorithms: %w", err) + } + algo, err := keyring.NewSigningAlgoFromString(string(cosmoshd.Ed25519Type), keyringAlgos) + if err != nil { + return nil, fmt.Errorf("failed to create signing algorithm: %w", err) + }
487-495: Improve peer configuration logic.The peer configuration logic could be more efficient:
- Consider pre-allocating the IPs slice
- The string join operation could be moved outside the loop
sort.Strings(addressesIPs) for i := 0; i < cfg.NumValidators; i++ { cfgTmp := network.Validators[i].Ctx.Config - IPs := make([]string, 0, cfg.NumValidators-1) + IPs := make([]string, 0, len(addressesIPs)-1) IPs = append(IPs, addressesIPs[0:i]...) IPs = append(IPs, addressesIPs[i+1:]...) cfgTmp.P2P.PersistentPeers = strings.Join(IPs, ",") tmcfg.WriteConfigFile(filepath.Join(cfgTmp.RootDir, "config", "config.toml"), cfgTmp) }
498-499: Consider making commission rate configurable.The commission rate is hardcoded to "0.5". Consider making this a configurable parameter in the Config struct.
+ // Add to Config struct + CommissionRate string // the validator commission rate (default: "0.5") - commissionRate, _ := sdkmath.LegacyNewDecFromStr("0.5") + commissionRate, err := sdkmath.LegacyNewDecFromStr(cfg.CommissionRate) + if err != nil { + return nil, fmt.Errorf("invalid commission rate: %w", err) + }
435-437: Clean up commented code.Remove commented out code that's no longer needed.
- // sdk.NewCoin(fmt.Sprintf("%stoken", nodeDirName), cfg.AccountTokens), - // sdk.NewCoin(cfg.BondDenom, cfg.DepositedTokens), sdk.NewCoin(cfg.NativeDenom, cfg.AccountTokens),tests/e2e/oracle/create_price.go (3)
155-157: ImplementTestCreatePriceNSTor add a TODO commentThe function
TestCreatePriceNSTis currently empty. Please provide an implementation or add aTODOcomment to indicate future work.Would you like assistance in writing the test cases for this function?
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 156-156:
File is notgofumpt-ed (gofumpt)
159-161: ImplementTestSlashingor add a TODO commentThe function
TestSlashingis currently empty. Please provide an implementation or add aTODOcomment to indicate future work.Would you like assistance in writing the test cases for this function?
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 160-160:
File is notgofumpt-ed (gofumpt)
156-156: Format the code usinggofumptLines 156 and 160 are not formatted according to
gofumptstandards. Please rungofumptto format the code.Also applies to: 160-160
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 156-156:
File is notgofumpt-ed (gofumpt)testutil/network/util.go (4)
44-44: Usemath.MaxUint64instead of hardcoded valueThe constant
MaxOptedOutHeightis defined as18446744073709551615. To improve readability and align with standard library constants, consider usingmath.MaxUint64for clarity.Apply this change:
-const MaxOptedOutHeight uint64 = 18446744073709551615 +const MaxOptedOutHeight uint64 = math.MaxUint64Don't forget to import the
mathpackage:+import ( + "math" + // other imports... +)
428-428: Consider integer overflow checks in power calculationAt line 428, the calculation:
power := sdk.TokensToConsensusPower(stakingAmount.Mul(sdkmath.NewInt(int64(len(genStateAssets.Tokens)))), evmostypes.PowerReduction)might risk integer overflow when
stakingAmountand the number of tokens are large.Consider adding overflow checks or using
sdkmath.Int'sOverflow()method to ensure safe calculations.
173-183: SetBaseFeedirectly fromcfg.MinGasPricesif possibleThe current logic slices
cfg.MinGasPricesand attempts to parse it into ansdkmath.Int. This approach assumes a specific format ofcfg.MinGasPrices.If
cfg.MinGasPricesis guaranteed to be in the correct format, consider simplifying the code:-var feemarketGenState feemarkettypes.GenesisState -cfg.Codec.MustUnmarshalJSON(cfg.GenesisState[feemarkettypes.ModuleName], &feemarketGenState) -l := len(cfg.MinGasPrices) - len(cfg.NativeDenom) -minGasPrice := sdkmath.ZeroInt() -if l > 0 { - minGasPriceStr := cfg.MinGasPrices[:l] - minGasPrice, _ = sdkmath.NewIntFromString(minGasPriceStr) -} -feemarketGenState.Params.BaseFee = minGasPrice +feemarketGenState := feemarkettypes.DefaultGenesisState() +feemarketGenState.Params.BaseFee = sdkmath.NewIntFromUint64(1)Ensure that this change aligns with your application's requirements.
289-347: Avoid modifying global variables in genesis state functionsFunctions like
NewGenStateAssetsmodifyDefaultGenStateAssets, which is a global variable. This practice can lead to unintended side effects and make the code harder to test and maintain.Refactor the function to create and return a new
GenesisStatewithout altering the global state:func NewGenStateAssets(operatorAccAddresses []sdk.AccAddress, depositAmount, stakingAmount sdkmath.Int) (assetstypes.GenesisState, error) { genStateAssets := assetstypes.GenesisState{ // initialize with default values or copy from DefaultGenStateAssets } // Modify genStateAssets instead of DefaultGenStateAssets // ... return genStateAssets, nil }This approach enhances modularity and testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
crypto/algo_ed25519.go(1 hunks)tests/e2e/bank/query.go(1 hunks)tests/e2e/oracle/create_price.go(1 hunks)tests/e2e/oracle/data.go(1 hunks)testutil/network/genesis_data.go(1 hunks)testutil/network/network.go(12 hunks)testutil/network/tx_internal.go(1 hunks)testutil/network/util.go(5 hunks)x/oracle/keeper/msg_server_update_params_test.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/bank/query.go
- tests/e2e/oracle/data.go
- testutil/network/tx_internal.go
- x/oracle/keeper/msg_server_update_params_test.go
🧰 Additional context used
📓 Learnings (2)
testutil/network/genesis_data.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: testutil/network/util.go:292-349
Timestamp: 2024-11-15T07:01:04.078Z
Learning: In the Go code for Exocore, specifically in the `testutil/network/util.go` file, functions like `NewGenStateAssets` are expected to modify the global variable `DefaultGenStateAssets`. Modifying global variables in these functions is acceptable and expected behavior in this context.
testutil/network/util.go (3)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: testutil/network/util.go:358-358
Timestamp: 2024-11-16T04:47:03.403Z
Learning: In test code, during initialization in `testutil/network/util.go`, unprotected integer multiplications like `stakingAmount` calculations are acceptable as the risk of integer overflow is minimal.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: testutil/network/util.go:309-309
Timestamp: 2024-11-16T04:41:59.771Z
Learning: In the `NewGenStateAssets` function in `testutil/network/util.go`, when generating `stakerAddrStr`, it is expected to use `hexutil.Encode(operatorAccAddress)` instead of `operatorAccAddress.String()`.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: testutil/network/util.go:40-40
Timestamp: 2024-11-15T06:58:30.450Z
Learning: In the Go file `testutil/network/util.go`, the import `evmostypes` (`github.com/evmos/evmos/v16/types`) is used for `evmostypes.PowerReduction` in the functions `NewGenStateOperator` and `NewGenStateDogfood`.
🪛 GitHub Check: Run golangci-lint
crypto/algo_ed25519.go
[failure] 44-44:
SA1019: ed25519.PrivKey is deprecated: PrivKey defines a ed25519 private key. NOTE: ed25519 keys must not be used in SDK apps except in a tendermint validator context. (staticcheck)
tests/e2e/oracle/create_price.go
[failure] 156-156:
File is not gofumpt-ed (gofumpt)
[failure] 160-160:
File is not gofumpt-ed (gofumpt)
[failure] 168-168:
func (*E2ETestSuite).moveNAndCheck is unused (unused)
🔇 Additional comments (5)
testutil/network/genesis_data.go (2)
1-10: LGTM: Clean and well-organized imports
The imports are properly organized and all necessary for the genesis state definitions.
53-58:
Add bounds checking for array access
The init function assumes the arrays in DefaultGenStateOracle.Params have the expected elements. Add bounds checking to prevent panic:
func init() {
+ if len(DefaultGenStateOracle.Params.Tokens) < 2 {
+ panic("Oracle genesis state requires at least 2 tokens")
+ }
+ if len(DefaultGenStateOracle.Params.TokenFeeders) < 2 {
+ panic("Oracle genesis state requires at least 2 token feeders")
+ }
DefaultGenStateOracle.Params.Tokens[1].AssetID = "0xdac17f958d2ee523a2206206994597c13d831ec7_0x65,0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee_0x65"
DefaultGenStateOracle.Params.TokenFeeders[1].StartBaseBlock = 10
}Likely invalid or redundant comment.
testutil/network/network.go (1)
116-131: Verify configuration values.
The default configuration values have been significantly modified:
- TimeoutCommit reduced from 3s to 2s
- MinGasPrices changed to "10hua"
- New token amounts introduced
testutil/network/util.go (2)
417-417: Ensure correct calculation in TokensToConsensusPower
The function sdk.TokensToConsensusPower is used with totalStakingAmount at line 417. Ensure that the calculation aligns with the expected consensus power, especially considering token denominations and potential overflows.
Run the following script to verify the calculation:
Replace ... with the expected value based on your test parameters.
✅ Verification successful
Let me gather more information about the TokensToConsensusPower function and PowerReduction to verify the calculation.
Let me check the BaseDenomUnit value and look for any tests that verify the TokensToConsensusPower calculation.
Based on the gathered information, I can now provide the final response:
TokensToConsensusPower calculation is correctly implemented
The calculation is correct and safe because:
- PowerReduction is properly defined with BaseDenomUnit of 18 (10^18)
- The code includes a comment explicitly stating "the price unit of assets is 1 not decimal 18"
- The conversion is consistently used across the codebase with the same PowerReduction parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Confirm that TokensToConsensusPower yields the expected value.
# Expected total staking amount
EXPECTED_TOTAL_STAKING_AMOUNT=...
# Check the consensus power calculation
grep -A 5 'TokensToConsensusPower' testutil/network/util.go
Length of output: 1126
Script:
#!/bin/bash
# Search for TokensToConsensusPower implementation and PowerReduction definition
rg "func TokensToConsensusPower" -A 5
# Search for PowerReduction constant in evmostypes
rg "PowerReduction" types/ -A 2
Length of output: 415
Script:
#!/bin/bash
# Search for BaseDenomUnit definition
rg "BaseDenomUnit.*=" -A 2
# Search for test cases involving TokensToConsensusPower
rg "TestTokensToConsensusPower|test.*TokensToConsensusPower" -A 5
# Look for any test files that use this calculation
rg "TokensToConsensusPower.*PowerReduction" testutil/ -B 2 -A 2
Length of output: 1982
379-388: Check for duplicate operator records
When appending to DefaultGenStateOperator.OperatorRecords, ensure that duplicate entries are not introduced, especially if initGenFiles can be called multiple times.
Run the following script to check for duplicates:
✅ Verification successful
No risk of duplicate operator records
The code is safe from duplicate operator records because:
DefaultGenStateOperatoris initialized as an empty state ingenesis_data.go- The
initGenFilesfunction is called only once during test network setup - Each operator record is added in a controlled loop over validators, where each validator has a unique address and consensus key
- The operator records are built from scratch each time, not appending to existing records
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that OperatorRecords have unique entries.
grep -A 5 'DefaultGenStateOperator.OperatorRecords = append' testutil/network/util.go
# Check for duplicates in the generated data
# This assumes you have a way to access the generated OperatorRecords
# For example, if you have output in JSON format:
jq '.OperatorRecords[] | .OperatorAddress' generated_genesis.json | sort | uniq -d
Length of output: 574
Script:
#!/bin/bash
# Let's analyze the code to understand the context and potential for duplicates
# First, let's see where DefaultGenStateOperator is defined and initialized
rg -A 5 "var DefaultGenStateOperator" testutil/network
# Then check the full implementation of the function where this append happens
ast-grep --pattern 'func $_ ($$$) {
$$$
DefaultGenStateOperator.OperatorRecords = append($$$)
$$$
}'
# Also check if there are any checks or cleanups before appending
rg -B 5 "DefaultGenStateOperator.OperatorRecords = append" testutil/network
Length of output: 570
Script:
#!/bin/bash
# Let's check the broader context of the file and initialization
# Check the full file content to understand the initialization and usage
cat testutil/network/util.go
# Also check if DefaultGenStateOperator is reset anywhere
rg "DefaultGenStateOperator.*=" testutil/network
Length of output: 19585
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
x/oracle/module.go (1)
196-196: Consider batching state updates and eventsThe code performs multiple state updates (reset context, update feeder IDs) with separate event emissions. Consider:
- Batching related state updates
- Emitting a single consolidated event
Example refactor:
- am.keeper.ResetAggregatorContextCheckTx() if feederIDs := am.keeper.GetUpdatedFeederIDs(); len(feederIDs) > 0 { feederIDsStr := strings.Join(feederIDs, "_") ctx.EventManager().EmitEvent(sdk.NewEvent( types.EventTypeCreatePrice, sdk.NewAttribute(types.AttributeKeyPriceUpdated, types.AttributeValuePriceUpdatedSuccess), sdk.NewAttribute(types.AttributeKeyFeederIDs, feederIDsStr), )) - am.keeper.ResetUpdatedFeederIDs() } + // Batch state updates + am.keeper.BatchReset(ctx) + // Emit consolidated event + if feederIDs := am.keeper.GetUpdatedFeederIDs(); len(feederIDs) > 0 { + ctx.EventManager().EmitEvent(createPriceUpdateEvent(feederIDs)) + }Also applies to: 209-216
tests/e2e/oracle/create_price.go (3)
46-74: Refactor duplicated code in test casesThe test cases
case_1tocase_4contain repetitive code patterns for setting up prices, creating messages, sending transactions, and querying results. Refactoring these sections can improve maintainability and reduce code duplication.Consider creating helper functions to handle repetitive tasks. For example:
func (s *E2ETestSuite) preparePrice(price *PriceStruct, tokenID string, round uint64) (oracletypes.PriceSource, error) { priceTest := price.updateTimestamp() priceTimeDetID := priceTest.getPriceTimeDetID(tokenID) priceSource := oracletypes.PriceSource{ SourceID: 1, Prices: []*oracletypes.PriceTimeDetID{ &priceTimeDetID, }, } return priceSource, nil } func (s *E2ETestSuite) sendPriceFromValidators(creators []sdk.AccAddress, msgs []*oracletypes.MsgCreatePrice, keys []string, kr []Keyring) error { for i, msg := range msgs { err := s.network.SendTxOracleCreateprice([]sdk.Msg{msg}, keys[i], kr[i]) if err != nil { return err } } return nil }Then, you can use these helpers in your test cases to reduce repetition.
Also applies to: 75-101, 102-134, 135-153
155-155: Add implementation or TODO comment forTestCreatePriceNSTThe test function
TestCreatePriceNSTis currently empty. If this test is intended to be implemented later, consider adding aTODOcomment to indicate future work.Would you like assistance in implementing this test or adding a
TODOcomment?
157-157: Add implementation or TODO comment forTestSlashingThe test function
TestSlashingis empty. To maintain code clarity, consider adding aTODOcomment or implementing the test if it's ready.Would you like help in implementing this test or adding a
TODOcomment?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
tests/e2e/bank/tx.go(1 hunks)tests/e2e/oracle/create_price.go(1 hunks)testutil/network/query.go(1 hunks)x/oracle/module.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/bank/tx.go
- testutil/network/query.go
🧰 Additional context used
🪛 GitHub Check: Run golangci-lint
tests/e2e/oracle/create_price.go
[failure] 164-164:
func (*E2ETestSuite).moveNAndCheck is unused (unused)
🔇 Additional comments (3)
x/oracle/module.go (3)
29-30: LGTM!
The module interface implementations are correctly declared.
162-165: Verify state consistency between caches and context
The retrieval of caches and aggregator context should be atomic to ensure consistency. Consider adding validation to ensure both are successfully retrieved before proceeding.
Let's check for similar patterns in the codebase:
#!/bin/bash
# Search for cache and context retrieval patterns
rg -A 10 "GetCaches.*GetAggregatorContext" --glob "x/oracle/**/*.go"156-157: Consider improving initialization strategy
While the current implementation works for the test framework, there are two concerns:
- Error values from
GetCaches()andGetAggregatorContext()are discarded - As noted in the TODO, initialization in BeginBlock may not be ideal
Consider:
- Moving initialization to
InitGenesisor module constructor - Adding error handling for production use
Let's verify the initialization pattern in other Cosmos SDK modules:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/e2e/oracle/create_price.go (3)
24-153: Consider refactoringTestCreatePriceLSTfor better readabilityThe
TestCreatePriceLSTfunction is quite lengthy and encompasses multiple test cases within a single function. Splitting it into smaller, focused test functions or utilizing sub-tests withs.Run()can enhance readability and maintainability.Apply this refactoring to introduce sub-tests:
func (s *E2ETestSuite) TestCreatePriceLST() { + s.Run("Case 1: Achieving supermajority with initial price submissions", func() { + // Original code for case 1 + }) + + s.Run("Case 2: Failing to reach supermajority", func() { + // Original code for case 2 + }) + + s.Run("Case 3: Reaching supermajority after additional submissions", func() { + // Original code for case 3 + }) + + s.Run("Case 4: Successful price update with all validators", func() { + // Original code for case 4 + }) }
37-84: Simplify repetitive price source initializationThe initialization of
priceSourceobjects is repetitive across different test cases. Consider creating a helper function to streamline this process and reduce code duplication.Introduce a helper function:
+func createPriceSource(priceTest PriceTest, detID string, sourceID uint64) oracletypes.PriceSource { + priceTimeDetID := priceTest.getPriceTimeDetID(detID) + return oracletypes.PriceSource{ + SourceID: sourceID, + Prices: []*oracletypes.PriceTimeDetID{ + &priceTimeDetID, + }, + } +} // Usage in case 1 -priceTest1R1 := price1.updateTimestamp() -priceTimeDetID1R1 := priceTest1R1.getPriceTimeDetID("9") -priceSource1R1 := oracletypes.PriceSource{ - SourceID: 1, - Prices: []*oracletypes.PriceTimeDetID{ - &priceTimeDetID1R1, - }, -} +priceTest1R1 := price1.updateTimestamp() +priceSource1R1 := createPriceSource(priceTest1R1, "9", 1) // Similar changes for other casesAlso applies to: 104-111, 138-141
155-157: Implement or remove empty test functionsThe functions
TestCreatePriceNSTandTestSlashingare empty. To maintain code clarity, either implement these test cases or remove them if they are not needed.Add TODO comments if these tests are planned for future implementation:
func (s *E2ETestSuite) TestCreatePriceNST() { + // TODO: Implement test cases for non-supermajority thresholds } func (s *E2ETestSuite) TestSlashing() { + // TODO: Implement slashing mechanism tests }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
crypto/algo_ed25519.go(1 hunks)tests/e2e/oracle/create_price.go(1 hunks)tests/e2e/oracle/data.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crypto/algo_ed25519.go
- tests/e2e/oracle/data.go
🧰 Additional context used
📓 Learnings (1)
tests/e2e/oracle/create_price.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: tests/e2e/oracle/create_price.go:37-39
Timestamp: 2024-11-16T11:07:01.278Z
Learning: In `tests/e2e/oracle/create_price.go`, variables like `price1` and `price2` may be defined in other files such as `tests/e2e/oracle/data.go` within the same PR. Ensure to check for variable definitions across all related files in the PR before flagging them as undefined.
testutil/network/tx_precompile.go
Outdated
| for precompileName, precompilePath := range precompilePaths { | ||
| p := filepath.Join(basePath, precompilePath) | ||
| abis[precompileName], err = parseABI(p) | ||
| if err != nil { | ||
| fmt.Printf("Failed to parse abi from path:%s\r\n", p) | ||
| return | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning test
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (16)
testutil/network/network.go (2)
84-90: Improve comment consistency for configuration fields.The comments for configuration fields should follow a consistent style. Some fields have comments while others don't.
Apply this diff to maintain consistency:
- AccountTokens sdkmath.Int // the amount of system tokens(e.g. 1000hua) - DepositedTokens sdkmath.Int - StakingTokens sdkmath.Int // the amount of tokens each validator stakes for every supporte asset + AccountTokens sdkmath.Int // The amount of system tokens (e.g. 1000hua) + DepositedTokens sdkmath.Int // The amount of tokens deposited into the system + StakingTokens sdkmath.Int // The amount of tokens each validator stakes for every supported asset
393-404: Enhance error handling in node initialization.The node initialization code could benefit from more descriptive error messages to aid in debugging test failures.
Consider wrapping errors with context:
-nodeID, pubKey, err := genutil.InitializeNodeValidatorFilesFromMnemonic(tmCfg, mnemonic) +nodeID, pubKey, err := genutil.InitializeNodeValidatorFilesFromMnemonic(tmCfg, mnemonic) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to initialize node %d validator files: %w", i, err) }tests/e2e/oracle/helper_nstconvert.go (6)
12-12: Typo in comment: 'alls takers' should be 'all stakers'.There's a minor typo in the comment at line 12. It should read "all stakers" instead of "alls takers".
16-17: Use more descriptive variable names for clarity.Variables like
strandindexcould be renamed to more descriptive names to enhance readability. For example:
- Rename
strtostakerBitmaskorbitStringto reflect that it represents a bitmask of staker indices.- Rename
indextocurrentStakerIndexto indicate its purpose in tracking the staker positions.
34-56: Simplify the bit length calculation and byte shifting logic.The logic for calculating
bitsand manipulatingchangeBytesis complex. Consider the following improvements:
- Use
bits = bits.Len(uint(change))from themath/bitspackage to simplify bit length calculation.- Refactor the byte shifting operations to make the code more concise and less error-prone.
This can enhance performance and maintainability.
59-68: Clarify the bit manipulation operations onbitsLengthBytes.The bitwise operations modifying
bitsLengthBytes[0]involve shifting and masking that may not be immediately clear. Adding comments to explain each operation will improve code readability and help future maintainers understand the intent.
88-123: Refactor the loop combiningchangeBytesListintochangeResultfor better readability.The loop from lines 91 to 123 is complex and handles intricate bit manipulations. Consider breaking this logic into smaller helper functions with descriptive names, or adding detailed comments to explain:
- The purpose of variables like
delta,left, and how they affect the concatenation of byte slices.- The conditions under which bytes are shifted or combined.
This will make the code easier to understand and maintain.
10-129: Add unit tests forconvertBalanceChangeToBytesto ensure correctness.Given the complexity of this function and its crucial role in data encoding, it's important to have comprehensive unit tests covering various scenarios, including edge cases and error conditions.
Would you like assistance in creating unit tests for this function?
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 10-10:
funcconvertBalanceChangeToBytesis unused (unused)testutil/network/tx_precompile.go (4)
135-135: Fix Typo in Function NamePaddnigAddressTo32There's a typo in the function name
PaddnigAddressTo32; it should bePaddingAddressTo32.Apply this diff to correct the function name and comment:
-// PaddnigAddressTo32 pads the 20-length address to 32 bytes with 0s -func PaddnigAddressTo32(address common.Address) []byte { +// PaddingAddressTo32 pads the 20-length address to 32 bytes with zeros +func PaddingAddressTo32(address common.Address) []byte {Also, ensure all calls to this function are updated accordingly.
47-48: Use Proper Error Handling Instead offmt.PrintlnUsing
fmt.Printlnfor error messages is not ideal. Consider returning the error or using a logging mechanism for better control over error handling.Apply this diff to return an error:
- fmt.Println("Failed to get current file path") - return + return fmt.Errorf("failed to get current file path")And update the
initfunction to handle the error:-func init() { +func init() { + err := initializePrecompiles() + if err != nil { + log.Fatalf("Initialization error: %v", err) + } +} -func initializePrecompiles() { +func initializePrecompiles() error {
106-106: Avoid Usingfmt.Printlnfor Logging Transaction IDDirectly printing to stdout is not recommended. Use a logging framework or return the transaction ID to the caller.
Apply this diff to use proper logging:
- fmt.Println("the txID is:", signTx.Hash().String()) + log.Println("Transaction ID:", signTx.Hash().String())Ensure the
logpackage is imported:import ( // other imports "log" )Alternatively, consider returning the transaction ID to the caller for better handling.
64-64: Use Pointer Receiver forNetworkMethodThe method
SendPrecompileTxhas a value receiverfunc (n Network). IfNetworkcontains fields that are expensive to copy or if you plan to modify its state, consider using a pointer receiver to improve performance.Apply this diff to change the receiver to a pointer:
-func (n Network) SendPrecompileTx(preCompileName precompile, methodName string, args ...interface{}) error { +func (n *Network) SendPrecompileTx(preCompileName precompile, methodName string, args ...interface{}) error {tests/e2e/oracle/create_price.go (4)
188-188: Typo in comment: 'automaticlly' should be 'automatically'There's a minor typographical error in the comment. Please correct 'automaticlly' to 'automatically'.
60-63: Refactor repeated code into helper functions to reduce duplicationThe code for creating messages and sending transactions is repeated multiple times across the test cases. Consider refactoring this code into helper functions to enhance maintainability and readability.
For example, you could create a helper function:
func (s *E2ETestSuite) sendCreatePrice(creator sdk.AccAddress, priceSource *oracletypes.PriceSource, height int64, valConsKey string, kr keyring.Keyring) error { msg := oracletypes.NewMsgCreatePrice(creator.String(), 1, []*oracletypes.PriceSource{priceSource}, height, 1) err := s.network.SendTxOracleCreateprice([]sdk.Msg{msg}, valConsKey, kr) return err }Then use this helper function in your test cases:
err := s.sendCreatePrice(creator0, &priceSource1R1, 10, "valconskey0", kr0) s.Require().NoError(err)Also applies to: 71-74, 76-79, 105-107, 129-131, 133-135, 137-139, 154-159
167-167: Add TODO comment to unimplemented test functionThe function
testCreatePriceNSTis currently empty. Adding a TODO comment can indicate its intended future implementation.Suggested change:
func (s *E2ETestSuite) testCreatePriceNST() { + // TODO: Implement test cases for creating NST prices }
170-170: Add TODO comment to unimplemented test functionThe function
testSlashingis currently empty. Adding a TODO comment can indicate its intended future implementation.Suggested change:
func (s *E2ETestSuite) testSlashing() { + // TODO: Implement slashing test scenarios }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
tests/e2e/oracle/create_price.go(1 hunks)tests/e2e/oracle/helper_nstconvert.go(1 hunks)testutil/network/network.go(12 hunks)testutil/network/tx_precompile.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
tests/e2e/oracle/create_price.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: tests/e2e/oracle/create_price.go:37-39
Timestamp: 2024-11-16T11:07:01.278Z
Learning: In `tests/e2e/oracle/create_price.go`, variables like `price1` and `price2` may be defined in other files such as `tests/e2e/oracle/data.go` within the same PR. Ensure to check for variable definitions across all related files in the PR before flagging them as undefined.
🪛 GitHub Check: Run golangci-lint
tests/e2e/oracle/helper_nstconvert.go
[failure] 10-10:
func convertBalanceChangeToBytes is unused (unused)
🪛 Gitleaks
testutil/network/tx_precompile.go
21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (3)
testutil/network/network.go (1)
487-495: Verify peer configuration impact.
The changes to peer configuration look correct, but we should verify that all validators can connect to each other properly.
✅ Verification successful
Let me gather more information about the peer configuration setup to ensure there are no conflicts or issues.
Based on the analysis of the shell script outputs, I can now provide a final response:
P2P configuration is correctly implemented
The peer configuration implementation is sound and follows best practices:
- The code correctly sets unique persistent peers for each validator by excluding self-connections
- P2P configurations are properly isolated per validator with no conflicting settings
- The implementation handles address management and peer connections appropriately through:
P2P.ListenAddressfor bindingP2P.AddrBookStrict = falseandP2P.AllowDuplicateIP = trueto allow test network flexibility- Proper persistent peer string formatting with
strings.Join(IPs, ",")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any potential issues with peer configuration
# Test: Search for P2P configuration patterns that might conflict with the new setup
# Check for any other P2P configuration that might interfere
rg -A 5 "P2P\.PersistentPeers.*=.*"
# Check for any hardcoded peer addresses that might conflict
rg "tcp://.*@.*:.*"
Length of output: 873
Script:
#!/bin/bash
# Check the network configuration setup in both files
cat testutil/network/network.go cmd/exocored/testnet.go | grep -A 10 -B 10 "P2P.PersistentPeers"
# Check for any other P2P related configurations
rg "P2P\." -A 3
# Look for any address or port configurations that might be related
rg "Address.*=.*:" -A 2
Length of output: 28818
tests/e2e/oracle/helper_nstconvert.go (2)
10-10: Function convertBalanceChangeToBytes is currently unused in the codebase.
According to static analysis, the function convertBalanceChangeToBytes is not used elsewhere. If this function is intended for future use or by reflection, please ensure it's correctly integrated. Otherwise, consider removing it to prevent dead code accumulation.
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 10-10:
func convertBalanceChangeToBytes is unused (unused)
28-32:
Potential off-by-one error when handling negative change amounts.
After converting negative change values to positive (line 30), the code decrements change by 1 (line 32). If change is -1, it becomes 1 after change *= -1, and then change-- makes it 0. This may lead to unintended behavior or incorrect encoding for small negative values. Please verify if decrementing change after making it positive is necessary and correct.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
testutil/network/tx_precompile.go (1)
28-28: Consider making gas limit configurableThe hardcoded gas limit of 500000 might not be suitable for all precompile operations. Consider making it configurable based on the operation type or accepting it as a parameter.
-const gasLimit = uint64(500000) +var DefaultGasLimit = uint64(500000)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
tests/e2e/oracle/helper_nstconvert.go(1 hunks)testutil/network/tx_precompile.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/oracle/helper_nstconvert.go
🧰 Additional context used
🪛 Gitleaks
testutil/network/tx_precompile.go
23-23: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
testutil/network/tx_precompile.go (1)
43-59: Improve error handling in init functionThe error messages could be more descriptive to aid in debugging:
func init() { _, currentFile, _, ok := runtime.Caller(0) if !ok { - fmt.Println("Failed to get current file path") - panic("Failed to get current file path") + panic("Failed to get current file path using runtime.Caller") } basePath := filepath.Dir(currentFile) var err error for precompileName, precompilePath := range precompilePaths { p := filepath.Join(basePath, precompilePath) abis[precompileName], err = parseABI(p) if err != nil { - panic(fmt.Sprintf("Failed to parse abi from path:%s\r\n", p)) + panic(fmt.Sprintf("Failed to parse ABI for precompile %s from path %s: %v", precompileName, p, err)) } } }🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 52-58: Iteration over map
Iteration over map may be a possible source of non-determinismtestutil/network/network.go (2)
52-58: Consider organizing imports for better maintainability.The commented out import
ekeyringon line 57 should be removed if it's no longer needed.-// ekeyring "github.com/evmos/evmos/v16/crypto/keyring"
425-428: Consider improving key seed file security.The key seed file is being written with default permissions. Consider adding more restrictive file permissions for better security.
-err = WriteFile(fmt.Sprintf("%v.json", "key_seed"), clientDir, infoBz) +err = WriteFile(fmt.Sprintf("%v.json", "key_seed"), clientDir, infoBz, 0600)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/e2e-test-release.yml(0 hunks).github/workflows/e2e-test.yml(0 hunks)tests/e2e/bank/tx.go(1 hunks)testutil/network/network.go(12 hunks)testutil/network/tx_precompile.go(1 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/e2e-test-release.yml
- .github/workflows/e2e-test.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/bank/tx.go
🧰 Additional context used
🪛 GitHub Check: CodeQL
testutil/network/tx_precompile.go
[notice] 9-9: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
[warning] 52-58: Iteration over map
Iteration over map may be a possible source of non-determinism
🔇 Additional comments (5)
testutil/network/tx_precompile.go (1)
124-139: LGTM! Helper functions are well-implemented
Both parseABI and PaddingAddressTo32 functions are correctly implemented with proper error handling and resource cleanup.
testutil/network/network.go (4)
484-492: Good improvement in peer address handling.
The sorting of addressesIPs and systematic assignment of persistent peers improves network stability and determinism.
84-91: Verify token denomination changes across the codebase.
The removal of BondDenom and addition of NativeDenom represents a significant change in token handling. This change needs careful verification across the codebase to ensure consistency.
#!/bin/bash
# Search for any remaining references to BondDenom
rg "BondDenom"
# Search for NativeDenom usage to ensure consistent implementation
rg "NativeDenom"
# Search for token denomination strings to verify consistency
rg -g '!*.md' '"hua"'499-505: 🛠️ Refactor suggestion
Avoid hardcoding gateway address.
The gateway address should be configurable rather than hardcoded. This allows for different test environments and improves maintainability.
Consider moving this to the Config struct:
+type Config struct {
+ // ... existing fields ...
+ GatewayAddress string
}
+func DefaultConfig() Config {
+ return Config{
+ // ... existing initialization ...
+ GatewayAddress: "exo18cggcpvwspnd5c6ny8wrqxpffj5zmhklprtnph",
+ }
}
-gateWayAddressStr := "exo18cggcpvwspnd5c6ny8wrqxpffj5zmhklprtnph"
+gateWayAddressStr := cfg.GatewayAddressLikely invalid or redundant comment.
116-123: Review token power reduction values.
The token amounts have been significantly reduced:
- AccountTokens: 1000 power
- DepositedTokens: 500 power
- StakingTokens: 200 power
These values seem quite low for a test network. Consider if these reductions might impact test scenarios.
testutil/network/tx_precompile.go
Outdated
| // SendPrecompileTx wraps the function to send a transaction to precompile contract with gateway address | ||
| // which defined in the test genesis state | ||
| func (n Network) SendPrecompileTx(preCompileName precompile, methodName string, args ...interface{}) error { | ||
| ctx := context.Background() | ||
|
|
||
| ethC := n.Validators[0].JSONRPCClient | ||
|
|
||
| chainID, err := ethC.ChainID(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get chainID, error:%w", err) | ||
| } | ||
|
|
||
| precompileAddr := precompileAddresses[preCompileName] | ||
| precompileABI := abis[preCompileName] | ||
|
|
||
| data, err := precompileABI.Pack(methodName, args...) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to pack message for %s, error:%w", methodName, err) | ||
| } | ||
|
|
||
| nonce, err := ethC.NonceAt(ctx, callAddr, nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| gasPrice, err := ethC.SuggestGasPrice(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| retTx := types.NewTx(&types.LegacyTx{ | ||
| Nonce: nonce, | ||
| To: &precompileAddr, | ||
| Value: big.NewInt(0), | ||
| Gas: gasLimit, | ||
| GasPrice: gasPrice, | ||
| Data: data, | ||
| }) | ||
| signer := types.LatestSignerForChainID(chainID) | ||
| signTx, err := types.SignTx(retTx, signer, sk) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| fmt.Println("the txID is:", signTx.Hash().String()) | ||
| msg := ethereum.CallMsg{ | ||
| From: callAddr, | ||
| To: retTx.To(), | ||
| Data: retTx.Data(), | ||
| } | ||
| _, err = ethC.CallContract(context.Background(), msg, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to call contract:%s, error:%w", preCompileName, err) | ||
| } | ||
|
|
||
| err = ethC.SendTransaction(ctx, signTx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to send transaction, error:%w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance transaction handling and error recovery
Several improvements can make this function more robust:
- Add transaction receipt handling to confirm success
- Use context with timeout
- Estimate gas instead of using fixed limit
- Improve error handling with more context
-func (n Network) SendPrecompileTx(preCompileName precompile, methodName string, args ...interface{}) error {
+func (n Network) SendPrecompileTx(ctx context.Context, preCompileName precompile, methodName string, args ...interface{}) (*types.Receipt, error) {
- ctx := context.Background()
+ if ctx == nil {
+ var cancel context.CancelFunc
+ ctx, cancel = context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
+ }
ethC := n.Validators[0].JSONRPCClient
chainID, err := ethC.ChainID(ctx)
if err != nil {
- return fmt.Errorf("failed to get chainID, error:%w", err)
+ return nil, fmt.Errorf("failed to get chainID: %w", err)
}
precompileAddr := precompileAddresses[preCompileName]
precompileABI := abis[preCompileName]
+ // Estimate gas instead of using fixed limit
+ msg := ethereum.CallMsg{
+ From: callAddr,
+ To: &precompileAddr,
+ Data: data,
+ }
+ gasLimit, err := ethC.EstimateGas(ctx, msg)
+ if err != nil {
+ return nil, fmt.Errorf("failed to estimate gas: %w", err)
+ }
+ // Add 20% buffer to gas estimate
+ gasLimit = gasLimit * 120 / 100
// ... rest of the function ...
err = ethC.SendTransaction(ctx, signTx)
if err != nil {
- return fmt.Errorf("failed to send transaction, error:%w", err)
+ return nil, fmt.Errorf("failed to send transaction to %s.%s: %w", preCompileName, methodName, err)
}
+ receipt, err := ethC.TransactionReceipt(ctx, signTx.Hash())
+ if err != nil {
+ return nil, fmt.Errorf("failed to get receipt for tx %s: %w", signTx.Hash().String(), err)
+ }
+
+ if receipt.Status == 0 {
+ return receipt, fmt.Errorf("transaction failed: %s", signTx.Hash().String())
+ }
+
- return nil
+ return receipt, nil
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
testutil/network/tx_precompile.go (1)
103-103: Replace fmt.Println with proper loggingDirect console output using
fmt.Printlnis not ideal for a test utility. Consider using a proper logging framework that can be configured based on the test environment.Example implementation:
-fmt.Println("the txID is:", signTx.Hash().String()) +log.Printf("Transaction sent with hash: %s", signTx.Hash().String())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
testutil/network/tx_precompile.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
testutil/network/tx_precompile.go
[notice] 9-9: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
[warning] 50-56: Iteration over map
Iteration over map may be a possible source of non-determinism
🔇 Additional comments (1)
testutil/network/tx_precompile.go (1)
122-137: LGTM! Helper functions are well-implemented
Both parseABI and PaddingAddressTo32 functions are correctly implemented with proper error handling and memory management.
leonz789
force-pushed
the
develop-e2e-test
branch
from
d70cfb9 to
c96883e
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (22)
testutil/network/tx.go (5)
18-18: Consider making gas adjustment configurableThe gas adjustment value of 1.5 is hardcoded. Consider making this configurable through network configuration to allow for different test scenarios and network conditions.
- txf, ctx, err := generateTxf(n.Validators[0].ClientCtx, keyName, keyring, 1.5, n.Config.MinGasPrices) + txf, ctx, err := generateTxf(n.Validators[0].ClientCtx, keyName, keyring, n.Config.GasAdjustment, n.Config.MinGasPrices)
27-27: Fix function name to follow Go naming conventionsThe function name has inconsistent capitalization. Rename from
SendTxOracleCreatepricetoSendTxOracleCreatePriceto follow Go naming conventions.
64-64: Add function documentationAdd a detailed godoc comment explaining the purpose, parameters, and return values of this helper function.
+// generateTxf creates and configures a transaction factory with the given parameters. +// It also updates the client context with the sender's address and name. +// Parameters: +// - ctx: The client context to be updated +// - keyName: The name of the key to be used for signing +// - kr: The keyring containing the keys +// - adjustment: Gas adjustment factor +// - minGasPrice: Minimum gas price to be used +// Returns the configured transaction factory, updated context, and any error encountered. func generateTxf(ctx client.Context, keyName string, kr keyring.Keyring, adjustment float64, minGasPrice string) (tx.Factory, client.Context, error) {
87-87: Consider making broadcast mode configurableThe broadcast mode is hardcoded to
BroadcastSync. Consider making this configurable through the network configuration to support different test scenarios.- ctx.BroadcastMode = flags.BroadcastSync + ctx.BroadcastMode = n.Config.BroadcastMode
25-26: Fix typos in commentsThere are several typos in the comments:
- "consturct" should be "construct"
- "senario" should be "scenario"
- "unexists" should be "non-existent"
x/oracle/keeper/keeper_suite_test.go (2)
76-80: Consider extracting duplicated parameter initializationThis parameter initialization code is duplicated in the SetupTest method. Consider extracting it into a helper method to improve maintainability.
+func (suite *KeeperSuite) initializeTestParams() { + p4Test := types.DefaultParams() + p4Test.TokenFeeders[1].StartBaseBlock = 1 + suite.k.SetParams(suite.ctx, p4Test) + suite.ctx = suite.ctx.WithBlockHeight(12) +} func (suite *KeeperSuite) Reset() { - p4Test := types.DefaultParams() - p4Test.TokenFeeders[1].StartBaseBlock = 1 - suite.k.SetParams(suite.ctx, p4Test) - suite.ctx = suite.ctx.WithBlockHeight(12) + suite.initializeTestParams() suite.ctrl = gomock.NewController(suite.t) resetSingle(suite.App.OracleKeeper) }
116-118: Consider a more descriptive function nameThe function name
resetSinglecould be more descriptive of its purpose. Consider renaming it to something likeresetKeeperStateorresetOracleStateto better reflect its functionality of resetting both the aggregator context and cache.-func resetSingle(k keeper.Keeper) { +func resetKeeperState(k keeper.Keeper) { k.ResetAggregatorContext() k.ResetCache() }x/oracle/keeper/msg_server_create_price_test.go (2)
85-87: Consider adding negative test casesThe test covers the happy path with three validators creating prices successfully. Consider adding test cases for:
- Invalid BasedBlock values
- Out of order messages
- Duplicate messages from the same validator
Also applies to: 97-99
Line range hint
28-46: Address TODO: Remove monkey patchingThe current test uses monkey patching with
gomonkeywhich can be fragile and maintenance-heavy. Consider:
- Using proper mocking with the existing mockgen setup
- Creating a proper test double for the dogfood keeper
Would you like help creating a proper mock implementation to replace the monkey patches?
testutil/network/tx_precompile.go (1)
44-47: Remove redundant error logging before panicThe code logs the error using
fmt.Printlnand then immediately panics with the same message. This is redundant as the panic message will be logged anyway.Apply this diff to remove the redundant logging:
if !ok { - fmt.Println("Failed to get current file path") panic("Failed to get current file path") }x/oracle/keeper/single.go (4)
23-56: Consider refactoring to reduce code duplication.The method contains similar initialization logic in both CheckTx and non-CheckTx paths. This could be refactored into a shared helper method to improve maintainability.
Consider refactoring like this:
func (k *Keeper) GetAggregatorContext(ctx sdk.Context) *aggregator.AggregatorContext { + func initNewContext() *aggregator.AggregatorContext { + c := k.GetCaches() + c.ResetCaches() + agc := aggregator.NewAggregatorContext() + if ok := k.recacheAggregatorContext(ctx, agc, c); !ok { + initAggregatorContext(ctx, agc, k, c) + } + return agc + } if ctx.IsCheckTx() { if k.memStore.agcCheckTx != nil { return k.memStore.agcCheckTx } if k.memStore.agc == nil { - c := k.GetCaches() - c.ResetCaches() - k.memStore.agcCheckTx = aggregator.NewAggregatorContext() - if ok := k.recacheAggregatorContext(ctx, k.memStore.agcCheckTx, c); !ok { - initAggregatorContext(ctx, k.memStore.agcCheckTx, k, c) - } + k.memStore.agcCheckTx = initNewContext() return k.memStore.agcCheckTx } k.memStore.agcCheckTx = k.memStore.agc.Copy4CheckTx() return k.memStore.agcCheckTx } if k.memStore.agc != nil { return k.memStore.agc } - c := k.GetCaches() - c.ResetCaches() - k.memStore.agc = aggregator.NewAggregatorContext() - if ok := k.recacheAggregatorContext(ctx, k.memStore.agc, c); !ok { - initAggregatorContext(ctx, k.memStore.agc, k, c) - } + k.memStore.agc = initNewContext() return k.memStore.agc }
Line range hint
59-157: Address TODO comment and consider breaking down the method.
- The TODO comment about hot fix needs attention:
// TODO: these 4 lines are mainly used for hot fixThis suggests technical debt that should be addressed.
- The method is handling multiple responsibilities (validator powers, parameter updates, message processing). Consider breaking it down into smaller, focused methods for better maintainability.
Would you like help creating a GitHub issue to track the TODO and/or assistance in breaking down this method into smaller, more focused methods?
Line range hint
160-182: Convert to Keeper method for consistency.This function should be a method on the Keeper struct to maintain consistency with other methods and improve encapsulation. Also consider adding comments explaining the power calculations.
-func initAggregatorContext(ctx sdk.Context, agc *aggregator.AggregatorContext, k *Keeper, c *cache.Cache) { +func (k *Keeper) initAggregatorContext(ctx sdk.Context, agc *aggregator.AggregatorContext, c *cache.Cache) { // ... rest of the function ... }
207-209: Simplify ResetUpdatedFeederIDs method.The condition check is unnecessary since setting nil is safe regardless of the current value.
func (k *Keeper) ResetUpdatedFeederIDs() { - if k.memStore.updatedFeederIDs != nil { - k.memStore.updatedFeederIDs = nil - } + k.memStore.updatedFeederIDs = nil }testutil/network/util.go (2)
261-269: Optimize genesis document initialization.The
InitialHeightandGenesisTimeare set inside the validator loop, but these values are the same for all validators.Move these initializations outside the loop:
gTime := cmttime.Now() +if genDoc.InitialHeight == 0 { + genDoc.InitialHeight = 1 +} +genDoc.GenesisTime = gTime + for i := 0; i < cfg.NumValidators; i++ { - if genDoc.InitialHeight == 0 { - genDoc.InitialHeight = 1 - } - genDoc.GenesisTime = gTime if err := genDoc.ValidateAndComplete(); err != nil { return err }
427-451: Add validation for power calculation.The power calculation could potentially result in a zero or negative value if
stakingAmountis too small.power := sdk.TokensToConsensusPower(stakingAmount.Mul(sdkmath.NewInt(int64(len(genStateAssets.Tokens)))), evmostypes.PowerReduction) +if power <= 0 { + return DefaultGenStateDogfood, fmt.Errorf("calculated power must be positive, got: %d", power) +}testutil/network/network.go (1)
84-91: Config struct field changes need documentation updatesThe changes to the Config struct fields need better documentation:
AccountTokenscomment should clarify these are native tokensDepositedTokensis missing a comment explaining its purposeStakingTokenscomment needs clarity on "supported asset"Add or update the comments as follows:
- AccountTokens sdkmath.Int // the amount of system tokens(e.g. 1000hua) + AccountTokens sdkmath.Int // the amount of native tokens each account receives initially (e.g. 1000hua) + DepositedTokens sdkmath.Int // the amount of tokens deposited into the network's liquidity pools StakingTokens sdkmath.Int // the amount of tokens each validator stakes for every supporte asset + StakingTokens sdkmath.Int // the amount of tokens each validator stakes for each supported asset typetestutil/network/tx_internal.go (2)
70-75: Initializetx.Factorywith proper settingsThe transaction factory is being initialized but lacks some configurations:
- Gas Adjustment: It's recommended to set the gas adjustment factor.
- Gas Prices: Set the gas prices to match network configurations.
- Sign Mode: Ensure the appropriate sign mode is set.
Consider updating the factory initialization:
txf := tx.Factory{}. WithChainID(ctx.ChainID). WithKeybase(kr). WithTxConfig(txConfig). WithAccountNumber(num). WithSequence(seq) + WithGasAdjustment(1.5). + WithGasPrices(n.Config.MinGasPrices). + WithSignMode(txConfig.SignModeHandler().DefaultMode())
77-79: Ensure single signature in transaction signingWhen signing the transaction, the
overwriteparameter is set totrue. This will overwrite any existing signatures. If multiple signers are needed in the future, this could cause issues.Consider setting
overwritetofalseif you plan to support multisig transactions.- if err := tx.Sign(txf, keyName, txBuilder, true); err != nil { + if err := tx.Sign(txf, keyName, txBuilder, false); err != nil {app/ante/cosmos/sigverify.go (3)
Line range hint
117-122: Potential Security Risk: Skipping Signature Gas ConsumptionIn the
SigGasConsumeDecorator, the gas consumption for signatures is skipped for oracle create-price transactions. This could lead to inaccurate gas accounting and potential exploitation, such as denial-of-service attacks through the submission of numerous transactions without incurring the appropriate gas costs.Ensure that gas consumption is properly accounted for all transaction types to prevent abuse and maintain network stability.
Line range hint
165-182: Potential Security Risk: Skipping Signature VerificationThe
SigVerificationDecoratorbypasses signature verification for oracle create-price transactions. This allows transactions to be processed without authenticating the sender's identity, posing a significant security risk by potentially accepting unauthorized or malicious transactions.All transactions should undergo signature verification to authenticate the sender and prevent unauthorized actions. Consider removing the bypass for oracle create-price transactions or implementing secure alternative validation mechanisms.
Line range hint
336-344: Inconsistent Sequence Handling inIncrementSequenceDecoratorThe
IncrementSequenceDecoratorskips incrementing the account sequence for oracle create-price transactions and instead relies on nonce handling viaCheckAndIncreaseNonce. This deviation from standard sequence management may lead to inconsistencies in transaction ordering and compromise replay protection mechanisms.Maintain consistent sequence management across all transaction types or ensure that the nonce mechanism provides equivalent security guarantees. If using nonces, thoroughly document and validate their implementation to prevent potential vulnerabilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (39)
.github/workflows/e2e-test-release.yml(0 hunks).github/workflows/e2e-test.yml(0 hunks)app/ante/cosmos/sigverify.go(1 hunks)crypto/algo_ed25519.go(1 hunks)go.mod(1 hunks)precompiles/assets/assets_test.go(0 hunks)tests/e2e/bank/bank_test.go(1 hunks)tests/e2e/bank/query.go(1 hunks)tests/e2e/bank/suite.go(1 hunks)tests/e2e/bank/tx.go(1 hunks)tests/e2e/basesuite.go(1 hunks)tests/e2e/oracle/create_price.go(1 hunks)tests/e2e/oracle/data.go(1 hunks)tests/e2e/oracle/helper_nstconvert.go(1 hunks)tests/e2e/oracle/oracle_test.go(1 hunks)tests/e2e/oracle/suite.go(1 hunks)tests/e2e/util.go(1 hunks)testutil/network/genesis_data.go(1 hunks)testutil/network/network.go(12 hunks)testutil/network/network_test.go(4 hunks)testutil/network/query.go(1 hunks)testutil/network/tx.go(1 hunks)testutil/network/tx_internal.go(1 hunks)testutil/network/tx_precompile.go(1 hunks)testutil/network/util.go(5 hunks)x/oracle/keeper/aggregator/aggregator.go(2 hunks)x/oracle/keeper/aggregator/calculator.go(1 hunks)x/oracle/keeper/aggregator/util.go(1 hunks)x/oracle/keeper/keeper.go(3 hunks)x/oracle/keeper/keeper_suite_test.go(2 hunks)x/oracle/keeper/msg_server_create_price.go(2 hunks)x/oracle/keeper/msg_server_create_price_test.go(5 hunks)x/oracle/keeper/msg_server_update_params.go(1 hunks)x/oracle/keeper/msg_server_update_params_test.go(7 hunks)x/oracle/keeper/nonce.go(2 hunks)x/oracle/keeper/params.go(2 hunks)x/oracle/keeper/prices.go(4 hunks)x/oracle/keeper/single.go(4 hunks)x/oracle/module.go(4 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/e2e-test-release.yml
- .github/workflows/e2e-test.yml
- precompiles/assets/assets_test.go
🚧 Files skipped from review as they are similar to previous changes (27)
- crypto/algo_ed25519.go
- go.mod
- tests/e2e/bank/bank_test.go
- tests/e2e/bank/query.go
- tests/e2e/bank/suite.go
- tests/e2e/bank/tx.go
- tests/e2e/basesuite.go
- tests/e2e/oracle/create_price.go
- tests/e2e/oracle/data.go
- tests/e2e/oracle/helper_nstconvert.go
- tests/e2e/oracle/oracle_test.go
- tests/e2e/oracle/suite.go
- tests/e2e/util.go
- testutil/network/genesis_data.go
- testutil/network/network_test.go
- testutil/network/query.go
- x/oracle/keeper/aggregator/aggregator.go
- x/oracle/keeper/aggregator/calculator.go
- x/oracle/keeper/aggregator/util.go
- x/oracle/keeper/keeper.go
- x/oracle/keeper/msg_server_create_price.go
- x/oracle/keeper/msg_server_update_params.go
- x/oracle/keeper/msg_server_update_params_test.go
- x/oracle/keeper/nonce.go
- x/oracle/keeper/params.go
- x/oracle/keeper/prices.go
- x/oracle/module.go
🧰 Additional context used
📓 Learnings (2)
testutil/network/tx_internal.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: testutil/network/tx_internal.go:21-21
Timestamp: 2024-11-16T08:56:12.141Z
Learning: In `testutil/network/tx_internal.go`, the `Validators` slice in the `Network` struct is guaranteed to have at least one validator, so it's unnecessary to check if `n.Validators` is non-empty before accessing `n.Validators[0]`.
testutil/network/util.go (3)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: testutil/network/util.go:358-358
Timestamp: 2024-11-16T04:47:03.403Z
Learning: In test code, during initialization in `testutil/network/util.go`, unprotected integer multiplications like `stakingAmount` calculations are acceptable as the risk of integer overflow is minimal.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: testutil/network/util.go:309-309
Timestamp: 2024-11-16T04:41:59.771Z
Learning: In the `NewGenStateAssets` function in `testutil/network/util.go`, when generating `stakerAddrStr`, it is expected to use `hexutil.Encode(operatorAccAddress)` instead of `operatorAccAddress.String()`.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: testutil/network/util.go:40-40
Timestamp: 2024-11-15T06:58:30.450Z
Learning: In the Go file `testutil/network/util.go`, the import `evmostypes` (`github.com/evmos/evmos/v16/types`) is used for `evmostypes.PowerReduction` in the functions `NewGenStateOperator` and `NewGenStateDogfood`.
🔇 Additional comments (24)
x/oracle/keeper/keeper_suite_test.go (3)
70-70: LGTM: Correctly updated resetSingle call
The update properly passes the OracleKeeper instance to match the new function signature.
82-82: LGTM: Correctly updated resetSingle call
The update properly passes the OracleKeeper instance to match the new function signature.
104-109: LGTM: Proper initialization sequence
The initialization sequence is correct:
- Resetting the keeper state
- Setting up the keeper and message server references
- Setting up the context
x/oracle/keeper/msg_server_create_price_test.go (1)
47-47: Verify block height and BasedBlock relationship
The test setup shows coordinated changes:
- Block height increased to 12
- BasedBlock increased to 11
- Cache access pattern updated to use App.OracleKeeper
Please verify if there's a specific requirement for the block height to be exactly one more than the BasedBlock. If this is a critical relationship, consider adding a comment explaining it.
Also applies to: 63-63, 67-68
✅ Verification successful
Let me analyze the results and generate additional verification scripts to understand the relationship better.
Based on the search results, I can now generate the final response since I have enough context to verify the relationship between block height and BasedBlock.
Block height and BasedBlock relationship is correct
The relationship between block height (12) and BasedBlock (11) is intentional and follows a consistent pattern in the oracle module where:
- BasedBlock represents the block on which price data is based
- Block height is always one block ahead of BasedBlock to allow for price data to be collected and processed
This is evidenced by:
- The pattern appears in multiple test files (msg_server_create_price_test.go, context_test.go)
- The oracle module's aggregator context explicitly uses this relationship for round preparation (x/oracle/keeper/aggregator/context_test.go)
- The relationship is used consistently across price creation messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test files that might establish this pattern
rg -A 2 "BasedBlock.*11" --type go
rg -A 2 "BlockHeight.*12" --type go
Length of output: 1638
Script:
#!/bin/bash
# Search for the test setup and context initialization
ast-grep --pattern 'func $_ {
$$$
ctx = ctx.WithBlockHeight($height)
$$$
}'
# Look for any comments or documentation about block height and based block relationship
rg -B2 -A2 "BasedBlock|BlockHeight" --type go --type md
Length of output: 66215
testutil/network/tx_precompile.go (2)
1-17: LGTM! Package structure and imports are well organized.
122-137: LGTM! Helper functions are well implemented.
Both parseABI and PaddingAddressTo32 functions are concise, correct, and handle their responsibilities appropriately.
x/oracle/keeper/single.go (2)
14-19: LGTM! Clean implementation of lazy initialization pattern.
The conversion to use memStore for caching is well implemented, following the lazy initialization pattern which helps with memory efficiency.
213-219: LGTM! Clean and straightforward implementation.
The getter and append methods for feederIDs are well implemented with appropriate type conversion.
testutil/network/util.go (6)
9-44: LGTM! Well-organized imports and constant definition.
The new imports are properly organized and necessary for the genesis state management functionality. The MaxOptedOutHeight constant is appropriately defined to replace magic numbers in the code.
208-248: LGTM! Well-structured genesis state initialization.
The initialization of validator-related modules is well-organized and follows a clear sequence:
- Collect validator addresses and public keys
- Initialize assets genesis state
- Initialize operator genesis state
- Initialize dogfood genesis state
- Initialize delegation genesis state
- Initialize oracle genesis state
Each step properly handles errors and maintains the chain of dependencies.
292-347: LGTM! Well-implemented assets genesis state generation.
The function properly validates inputs and handles the initialization of deposits and operator assets. The use of DefaultGenStateAssets is acceptable in test code as per the learnings.
453-481: LGTM! Well-structured delegation genesis state generation.
The function properly handles the initialization of delegation states and associations. The use of maps to track linked staker IDs is an efficient approach.
483-485: LGTM! Simple oracle genesis state initialization.
The function provides a clean interface to initialize oracle genesis state using the default values.
354-422: 🛠️ Refactor suggestion
Replace magic number with MaxOptedOutHeight constant.
The function uses a magic number 18446744073709551615 for OptedOutHeight instead of the defined constant.
- OptedOutHeight: 18446744073709551615,
+ OptedOutHeight: MaxOptedOutHeight,Likely invalid or redundant comment.
testutil/network/network.go (4)
484-492: Good addition of sorted persistent peers
The addition of sorted persistent peers is a good improvement that ensures consistent network topology across test runs.
499-505: 🛠️ Refactor suggestion
Avoid hardcoding gateway address
The gateway address should not be hardcoded as it reduces flexibility and makes testing different scenarios difficult.
Move this to the Config struct:
type Config struct {
// ... existing fields ...
+ GatewayAddress string // The address of the gateway account
}
func DefaultConfig() Config {
// ... existing initialization ...
+ GatewayAddress: "exo18cggcpvwspnd5c6ny8wrqxpffj5zmhklprtnph",
}
-gateWayAddressStr := "exo18cggcpvwspnd5c6ny8wrqxpffj5zmhklprtnph"
+gateWayAddressStr := cfg.GatewayAddressLikely invalid or redundant comment.
380-394: 🛠️ Refactor suggestion
Enhance key generation security
The key generation process could be improved:
- Using a fixed English language for mnemonics reduces entropy
- No entropy source is specified for mnemonic generation
Consider enhancing the key generation:
- _, mnemonic, err := kb.NewMnemonic(fmt.Sprintf("valconskey%d", i), keyring.English, sdk.GetConfig().GetBech32AccountPubPrefix(), keyring.DefaultBIP39Passphrase, algo)
+ entropy, err := bip39.NewEntropy(256)
+ if err != nil {
+ return nil, err
+ }
+ _, mnemonic, err := kb.NewMnemonic(
+ fmt.Sprintf("valconskey%d", i),
+ keyring.English,
+ sdk.GetConfig().GetBech32AccountPubPrefix(),
+ keyring.DefaultBIP39Passphrase,
+ algo,
+ entropy,
+ )Likely invalid or redundant comment.
116-123: Review default token allocations
The token allocation values seem quite low for a test network:
- AccountTokens: 1000 power
- DepositedTokens: 500 power
- StakingTokens: 200 power
These values might not be sufficient for comprehensive testing scenarios, especially for operations requiring significant liquidity.
Let's verify if these values align with other test configurations:
✅ Verification successful
Let me search for more context about token allocations in test configurations.
Let me search for any test failures or issues related to token amounts to verify if the current values are causing any problems.
Based on the gathered evidence, I can now provide a final response:
Token allocations are appropriate for test scenarios
The current token allocations (1000, 500, and 200 power) appear to be sufficient for testing purposes as:
- These values are consistently used across test configurations
- No test failures related to insufficient balances were found
- The e2e tests successfully validate balances with these amounts
- The values maintain appropriate proportions between account, deposit, and staking operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other token allocation patterns in test files
rg -A 2 "TokensFromConsensusPower.*evmostypes.PowerReduction" testutil/
Length of output: 742
Script:
#!/bin/bash
# Search for other test configurations and token allocations
rg -B 2 -A 2 "AccountTokens|StakingTokens|DepositedTokens" tests/ testutil/
Length of output: 4287
Script:
#!/bin/bash
# Search for test failures or issues related to token amounts
rg -B 2 -A 2 "insufficient.*balance|not enough|token.*fail" tests/
Length of output: 68
testutil/network/tx_internal.go (6)
25-25: Check for empty Validators slice before accessing index 0
Similar to the previous comment, ensure that n.Validators[0] is safely accessed.
Since this concern was previously raised, and the user indicated that there's always at least one validator, we can proceed with caution. However, for future-proofing, consider adding a check.
42-45: Handle error returned by TxEncoder when encoding the transaction
The error from txConfig.TxEncoder()(txBuilder.GetTx()) is being correctly handled here. Good job on ensuring that potential errors are not ignored.
46-52: Properly handle errors from simulation
The simulation step correctly checks and handles errors returned by txClient.Simulate. This ensures that any issues during gas estimation are caught early.
81-84: Handle error when encoding the signed transaction
Good job handling the error returned when encoding the signed transaction. This ensures any issues during encoding are caught.
86-86: Return the signed transaction bytes
The function correctly returns the signed transaction bytes.
14-21: Ensure the existence of validators before accessing index 0
Accessing n.Validators[0] without checking if n.Validators is non-empty can lead to a runtime panic if the slice is empty. Even though previous learning indicates that Validators is guaranteed to have at least one validator, it's safer to include a check to prevent potential issues if future changes alter this assumption.
Run the following script to confirm that Validators is always populated:
| resSigningInfo, err := s.network.QuerySlashing().SigningInfo(context.Background(), &slashingtypes.QuerySigningInfoRequest{ConsAddress: sdk.ConsAddress(s.network.Validators[3].PubKey.Address()).String()}) | ||
| s.Require().NoError(err) | ||
| // validator3 is jailed | ||
| s.Require().True(resSigningInfo.ValSigningInfo.JailedUntil.After(time.Now())) |
Check warning
Code scanning / CodeQL
Calling the system time Warning test
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (27)
tests/e2e/oracle/suite.go (3)
10-15: Consider improving field encapsulation.The
networkfield could be made private (renamed tonetwork) since it's an implementation detail. If external access is needed, consider adding getter methods.type E2ETestSuite struct { suite.Suite cfg network.Config - network *network.Network + network *network.Network // private field for better encapsulation }
21-28: Consider making timeout and target height configurable.The hardcoded values for timeout (20 seconds) and target height (2) could be made configurable through constants or suite fields. This would allow for different test environments that might need longer timeouts or higher block heights.
+const ( + defaultSetupTimeout = 20 * time.Second + defaultTargetHeight = 2 +) + func (s *E2ETestSuite) SetupSuite() { s.T().Log("setting up e2e test suite") var err error s.network, err = network.New(s.T(), s.T().TempDir(), s.cfg) s.Require().NoError(err) - _, err = s.network.WaitForHeightWithTimeout(2, 20*time.Second) + _, err = s.network.WaitForHeightWithTimeout(defaultTargetHeight, defaultSetupTimeout) s.Require().NoError(err) }
30-32: Enhance teardown with logging and error handling.Consider adding logging and error handling to ensure proper cleanup and help with debugging test failures.
func (s *E2ETestSuite) TearDownSuite() { + s.T().Log("tearing down e2e test suite") s.network.Cleanup() + s.T().Log("cleanup completed") }x/oracle/types/types.go (2)
Line range hint
19-23: Clean up commented-out codeThe struct definitions contain commented-out fields that should be completely removed rather than left as comments. This improves code readability and maintenance.
Apply this diff to clean up the commented-out code:
Token struct { Name string `json:"name"` Desc string - // Chain struct { - // Name string `json:"name"` - // Desc string `json:"desc"` - // } `json:"chain"` Decimal string `json:"decimal"` Contract string `json:"contract"` AssetID string `json:"asset_id"` } `json:"token"` Feeder struct { - // Start string `json:"start"` - // End string `json:"end"` Interval string `json:"interval"` - // RuleID string `json:"rule_id"` } `json:"feeder"`Also applies to: 26-28
49-50: Document the purpose of NSTIDPrefix constantThe new constant
NSTIDPrefixlacks documentation explaining its purpose and usage. Since it's used for Non-Standard Token identification, this should be clearly documented.Add documentation comment:
+// NSTIDPrefix defines the prefix used for Non-Standard Token identifiers const ( DefaultPriceValue = 1 DefaultPriceDecimal = 0 NSTIDPrefix = "NST" )testutil/network/query.go (1)
13-39: Consider refactoring to reduce code duplication.All query methods follow the same pattern, differing only in the type-specific
NewQueryClientfunction. Consider introducing a generic helper to reduce duplication.Here's a suggested approach using generics:
// Generic helper function func (n *Network) queryClient[T any](newClient func(client.Context) T) T { if len(n.Validators) == 0 { panic("network has no validators") } return newClient(n.Validators[0].ClientCtx) } // Example usage for one method func (n *Network) QueryOracle() oracletypes.QueryClient { return n.queryClient(oracletypes.NewQueryClient) }tests/e2e/oracle/data.go (2)
9-9: Consider using RFC3339 time format.The custom time format could lead to parsing issues. Consider using the standard RFC3339 format for better interoperability.
-const layout = "2006-01-02 15:04:05" +const layout = time.RFC3339
41-41: Specify reasons for linting suppressions.The
//nolint:alldirective suppresses all linting checks without specifying why. Consider using specific linting suppressions with explanatory comments.-//nolint:all +// nolint:unused // sourceID is required for interface compatibilitytestutil/network/genesis_data.go (2)
15-24: Improve constant naming and documentation.While the constants are documented, consider these improvements:
- Use a more descriptive prefix for test-specific constants (e.g.,
TEST_EVM_CHAIN_ID).- Add documentation for
NativeAssetAddress,ETHAssetAddress,NativeAssetID, andETHAssetID.- Consider grouping related constants together (chain-related, address-related, ID-related).
const ( - // TestEVMChainID represents the LayerZero chain ID for the test EVM chain - TestEVMChainID = 101 - // EVMAddressLength is the standard length of EVM addresses in bytes - EVMAddressLength = 20 - NativeAssetAddress = "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" - ETHAssetAddress = "0xdac17f958d2ee523a2206206994597c13d831ec7" - NativeAssetID = "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee_0x65" - ETHAssetID = "0xdac17f958d2ee523a2206206994597c13d831ec7_0x65" + // Chain-related constants + TEST_EVM_CHAIN_ID = 101 // LayerZero chain ID for the test EVM chain + + // Address-related constants + EVM_ADDRESS_LENGTH = 20 // Standard length of EVM addresses in bytes + // Test addresses for assets (DO NOT USE IN PRODUCTION) + TEST_NATIVE_ASSET_ADDRESS = "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" + TEST_ETH_ASSET_ADDRESS = "0xdac17f958d2ee523a2206206994597c13d831ec7" + + // Asset ID constants for testing + TEST_NATIVE_ASSET_ID = TEST_NATIVE_ASSET_ADDRESS + "_0x65" + TEST_ETH_ASSET_ID = TEST_ETH_ASSET_ADDRESS + "_0x65" )
49-54: Remove commented-out code.The commented-out code for
DefaultGenStateOperatorandDefaultGenStateDelegationshould either be used or removed to maintain code clarity.x/oracle/keeper/aggregator/calculator.go (4)
19-20: Consider type consistency between price and powerThe struct now mixes string (price) and *big.Int (power) types. This could lead to confusion in operations and comparisons. Consider:
- Documenting the expected string format for price
- Adding helper methods for price comparisons
27-29: Remove or document the commented fieldThe commented
confirmedfield should either be removed if no longer needed or documented if planned for future use.
Line range hint
179-184: Address TODO comments and document version constraintsThe code contains multiple TODO comments indicating temporary solutions and future improvements:
- "just in v1 with mode==1"
- "this may delay to current block finish"
These should be tracked and addressed in future updates.
Would you like me to create GitHub issues to track these TODO items?
Line range hint
1-197: Consider architectural implications of string-based price representationThe transition from
*big.Intto string for price representation has several architectural implications:
- Performance: String operations vs numerical operations
- Precision: Potential loss of precision in string representation
- Validation: Need for consistent string format validation
- Interoperability: Impact on external systems and APIs
Consider documenting these design decisions and their rationale.
x/oracle/keeper/aggregator/aggregator.go (3)
27-41: Consider logging non-standard price formats.The early return for non-standard prices (NST) is a reasonable approach, but consider adding logging to track such occurrences. This would help monitor the frequency of non-standard price submissions.
if !ok { + // TODO: Add logging for non-standard price formats return p.price }
59-61: Consider using a string pool for frequently used price strings.While string copying is simpler than big.Int copying, consider using a string pool for frequently occurring price values to reduce memory allocations, especially in high-throughput scenarios.
Also applies to: 71-72, 77-77
Line range hint
171-195: Document consensus implications for non-standard prices.The early return for non-standard prices when power exceeds threshold could affect consensus mechanism. Consider:
- Documenting this behavior in comments
- Adding metrics to track frequency of non-standard price consensus
- Implementing a more robust consensus mechanism for mixed price formats
x/oracle/module.go (1)
Line range hint
357-377: Consider optimizing cache operationsThe cache operations and event emissions could be optimized and made more maintainable.
Consider extracting the cache and event handling logic into separate methods:
+func (am AppModule) handleCacheUpdates(ctx sdk.Context, cs cache.Caches, agc types.AggregatorContext) error { + if _, _, paramsUpdated := cs.CommitCache(ctx, false, am.keeper); paramsUpdated { + var p cache.ItemP + if err := cs.GetCache(&p); err != nil { + return err + } + params := types.Params(p) + agc.SetParams(¶ms) + ctx.EventManager().EmitEvent(sdk.NewEvent( + types.EventTypeCreatePrice, + sdk.NewAttribute(types.AttributeKeyParamsUpdated, types.AttributeValueParamsUpdatedSuccess), + )) + } + return nil +}x/oracle/keeper/native_token.go (2)
26-33: Consider extracting hardcoded values to configurationThe constants contain hardcoded Ethereum addresses and chain IDs. This could make it difficult to update values or add support for new networks.
Consider:
- Moving these values to a configuration file or genesis parameters
- Creating a more flexible structure to support different asset types beyond NSTETH
-const ( - NSTETHAssetAddr = "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" - NSTETHAssetIDMainnet NSTAssetID = "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee_0x7595" - NSTETHAssetIDLocalnet NSTAssetID = "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee_0x65" - NSTETHAssetIDHolesky NSTAssetID = "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee_0x9d19" - NSTETHAssetIDSepolia NSTAssetID = "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee_0x9ce1" -) +var ( + networkConfig = types.NetworkConfig{ + NSTETHAssetAddr: "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", + Networks: map[string]types.NetworkInfo{ + "mainnet": {ChainID: "0x7595", Address: "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee"}, + "localnet": {ChainID: "0x65", Address: "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee"}, + "holesky": {ChainID: "0x9d19", Address: "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee"}, + "sepolia": {ChainID: "0x9ce1", Address: "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee"}, + }, + } +)
28-29: Address TODO comments about NSTETH limitationsThe TODO comments indicate limitations in the current implementation:
- Only NSTETH is supported with capped effective balance
- The endpoint versioning approach is noted as "bad practice"
Would you like me to help create GitHub issues to track:
- Support for additional asset types beyond NSTETH
- Improved endpoint versioning strategy
testutil/network/network.go (2)
484-492: Add error handling for peer setupThe peer setup logic is good but could be more robust:
- Consider handling edge cases (e.g., when no peers are available)
- Add validation for the peer addresses format
sort.Strings(addressesIPs) +if len(addressesIPs) == 0 { + return nil, errors.New("no peer addresses available") +} for i := 0; i < cfg.NumValidators; i++ { cfgTmp := network.Validators[i].Ctx.Config IPs := make([]string, 0, cfg.NumValidators-1) IPs = append(IPs, addressesIPs[0:i]...) IPs = append(IPs, addressesIPs[i+1:]...) + for _, ip := range IPs { + if !strings.Contains(ip, "@") { + return nil, fmt.Errorf("invalid peer address format: %s", ip) + } + } cfgTmp.P2P.PersistentPeers = strings.Join(IPs, ",")
582-633: Improve documentation and reduce duplicationThe state height methods are well-implemented but need improvements:
- Add detailed documentation explaining the difference between block height and state height
- Consider reducing code duplication with block height methods
+// LatestStateHeight returns the latest state height from ABCI info. +// Unlike LatestHeight which returns the latest block height, +// this method returns the height at which all state transitions are completed. func (n *Network) LatestStateHeight() (int64, error) {Consider extracting common waiting logic:
+func (n *Network) waitForHeightWithTimeoutAndQuery(h int64, t time.Duration, queryFn func() (int64, error)) (int64, error) { + ticker := time.NewTicker(time.Second) + timeout := time.After(t) + + if len(n.Validators) == 0 { + return 0, errors.New("no validators available") + } + + var latestHeight int64 + for { + select { + case <-timeout: + ticker.Stop() + return latestHeight, errors.New("timeout exceeded waiting for block") + case <-ticker.C: + height, err := queryFn() + if err == nil { + latestHeight = height + if latestHeight >= h { + return latestHeight, nil + } + } + } + } +}x/delegation/keeper/update_native_restaking_balance.go (2)
Line range hint
65-65: Fix typo in comment: 'ture' should be 'true'There's a minor typo in the comment that should be corrected for clarity.
Apply the following diff:
- // return ture to break the iteration if there isn't remaining amount to be slashed + // return true to break the iteration if there isn't remaining amount to be slashed
Line range hint
76-80: Avoid variable shadowing oferrin closure functionsIn the
opFuncfunctions, the variableerris redeclared using:=, which shadows the outererrvariable. This can lead to confusion and potential bugs. It's recommended to use=when assigning toerrif it has already been declared.Apply the following diff to fix the variable shadowing:
- opAccAddr, err := sdk.AccAddressFromBech32(keys.OperatorAddr) + opAccAddr, err = sdk.AccAddressFromBech32(keys.OperatorAddr)Alternatively, declare a new variable name:
- opAccAddr, err := sdk.AccAddressFromBech32(keys.OperatorAddr) + opAccAddr, addrErr := sdk.AccAddressFromBech32(keys.OperatorAddr) + if addrErr != nil { + return true, addrErr + }tests/e2e/oracle/create_price.go (3)
106-106: Reminder: Address the TODO regarding block height varianceThe TODO comment suggests that the block height may exceed 13 and recommends using a larger price window (
nonce > 3) to increase confidence. Addressing this will enhance test reliability.Would you like assistance in adjusting the test parameters or updating the price window to accommodate this scenario?
334-334: Remove debug print statements from committed codeThe statement
fmt.Println("debug----->jailed:", resOperator.Jailed)appears to be for debugging purposes. It's best practice to remove such statements or replace them with proper logging before committing code.
290-297: Refactor repeated code for creating and sending price messagesThe code for creating and sending price messages for multiple validators is duplicated. Consider refactoring this into a helper function to improve maintainability and reduce redundancy.
You could create a function like
sendPriceMessages(validators, priceSource, height, nonce)to encapsulate this logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
tests/e2e/oracle/create_price.go(1 hunks)tests/e2e/oracle/data.go(1 hunks)tests/e2e/oracle/helper_nstconvert.go(1 hunks)tests/e2e/oracle/suite.go(1 hunks)testutil/network/genesis_data.go(1 hunks)testutil/network/network.go(15 hunks)testutil/network/query.go(1 hunks)x/delegation/keeper/update_native_restaking_balance.go(1 hunks)x/oracle/keeper/aggregator/aggregator.go(9 hunks)x/oracle/keeper/aggregator/calculator.go(5 hunks)x/oracle/keeper/aggregator/context.go(4 hunks)x/oracle/keeper/aggregator/worker.go(2 hunks)x/oracle/keeper/native_token.go(3 hunks)x/oracle/keeper/prices.go(5 hunks)x/oracle/module.go(5 hunks)x/oracle/types/params.go(2 hunks)x/oracle/types/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/oracle/helper_nstconvert.go
- x/oracle/keeper/prices.go
🧰 Additional context used
📓 Learnings (3)
tests/e2e/oracle/create_price.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: tests/e2e/oracle/create_price.go:37-39
Timestamp: 2024-11-16T11:07:01.278Z
Learning: In `tests/e2e/oracle/create_price.go`, variables like `price1` and `price2` may be defined in other files such as `tests/e2e/oracle/data.go` within the same PR. Ensure to check for variable definitions across all related files in the PR before flagging them as undefined.
tests/e2e/oracle/data.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: tests/e2e/oracle/create_price.go:37-39
Timestamp: 2024-11-16T11:07:01.278Z
Learning: In `tests/e2e/oracle/create_price.go`, variables like `price1` and `price2` may be defined in other files such as `tests/e2e/oracle/data.go` within the same PR. Ensure to check for variable definitions across all related files in the PR before flagging them as undefined.
testutil/network/genesis_data.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: testutil/network/util.go:292-349
Timestamp: 2024-11-15T07:01:04.078Z
Learning: In the Go code for Exocore, specifically in the `testutil/network/util.go` file, functions like `NewGenStateAssets` are expected to modify the global variable `DefaultGenStateAssets`. Modifying global variables in these functions is acceptable and expected behavior in this context.
🪛 GitHub Check: CodeQL
tests/e2e/oracle/create_price.go
[warning] 319-319: Calling the system time
Calling the system time may be a possible source of non-determinism
tests/e2e/oracle/data.go
[warning] 36-36: Calling the system time
Calling the system time may be a possible source of non-determinism
[warning] 57-57: Calling the system time
Calling the system time may be a possible source of non-determinism
[warning] 71-71: Calling the system time
Calling the system time may be a possible source of non-determinism
x/oracle/module.go
[warning] 160-160: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
[warning] 163-165: Iteration over map
Iteration over map may be a possible source of non-determinism
🔇 Additional comments (22)
tests/e2e/oracle/suite.go (1)
1-32: Verify consistency with other e2e test suites.
Let's ensure this suite structure follows the same patterns as other e2e test suites in the project.
✅ Verification successful
Based on the extensive output from examining the test suites in the codebase, I can now provide a definitive response about the consistency of the e2e test suite structure.
E2E test suite structure is consistent with project patterns
The oracle e2e test suite follows the same patterns used in other e2e test suites in the project, specifically:
- It properly embeds
suite.Suite - It contains the standard network configuration and setup fields:
cfg network.Confignetwork *network.Network
- It implements the required suite lifecycle methods:
SetupSuite()for initializationTearDownSuite()for cleanup
- It follows the same network setup pattern seen in other e2e suites like
bank/suite.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency with other e2e test suites
# Look for similar suite structures in other e2e tests
# Search for other e2e test suites
ast-grep --pattern 'type $_TestSuite struct {
suite.Suite
$$$
}'
# Search for other SetupSuite implementations
ast-grep --pattern 'func ($_ *$_TestSuite) SetupSuite() {
$$$
}'
Length of output: 145
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find test suites
# Search for test suite struct definitions
rg -l "type.*TestSuite.*struct" --type go
# Then let's examine the content of these files
for file in $(rg -l "type.*TestSuite.*struct" --type go); do
echo "=== $file ==="
cat "$file"
echo -e "\n\n"
done
Length of output: 104111
x/oracle/types/types.go (1)
50-50: Verify the integration of NSTIDPrefix
Let's ensure this new prefix is properly integrated and doesn't conflict with existing token identifiers.
✅ Verification successful
Let me gather more information about the usage of this prefix and related functionality.
Based on the search results, I can now provide a final response about the NSTIDPrefix integration:
NSTIDPrefix is properly integrated and used consistently
The prefix "NST" is well-integrated into the codebase and is used consistently for native staking token functionality:
- Used in price updates via
UpdateNSTByBalanceChange - Properly validated through
IsNSTchecks - Integrated with token systems through
DepositNSTandWithdrawNSToperations - No conflicts found with other ID prefixes (only other prefix found is
ChainIDPrefixwhich serves a different purpose)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of NSTIDPrefix and similar prefix constants
echo "Searching for NSTIDPrefix usage and other ID prefixes..."
rg --type go "IDPrefix.*=.*\".*\""
# Search for potential prefix-related functions
echo "Searching for prefix-related functions..."
ast-grep --pattern 'func $$$($$$) {
$$$
NST$$$
$$$
}'
Length of output: 421
Script:
#!/bin/bash
# Search for any NST related code or strings
echo "Searching for NST related code..."
rg --type go "NST"
# Search for functions that might be generating or validating IDs
echo "Searching for ID generation/validation functions..."
rg --type go "func.*(Generate|Valid|New).*ID"
# Search for test files that might be using this prefix
echo "Searching in test files..."
rg --type go -t test "NST"
Length of output: 18316
testutil/network/query.go (1)
1-11: LGTM! Clean import organization.
The imports are well-organized and properly grouped between Exocore-specific and Cosmos SDK types.
tests/e2e/oracle/data.go (1)
11-15: LGTM! Well-structured data type.
The struct is well-defined with appropriate field types. Using string for Price allows handling arbitrary precision values.
x/oracle/keeper/aggregator/worker.go (2)
98-100: Verify the impact of return type change from *big.Int to string
The change in return type is a breaking change that could affect consumers of this API. Let's verify all usages to ensure they've been updated accordingly.
✅ Verification successful
Let me gather more information about the usage context.
Let me check one more thing to understand how the worker interacts with the aggregator.
Based on the comprehensive analysis of the code, I can now provide the final response:
The return type change from *big.Int to string is properly handled throughout the codebase
The change is part of a consistent design where:
- The worker's
aggregate()method simply delegates to the underlying aggregator - The aggregator handles all price values as strings internally
- The conversion between
*big.Intand string happens only when needed for mathematical operations (median calculation) - All price-related structures (
priceWithTimeAndRound,reportPrice,aggregator) are already using string representation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to worker.aggregate() to verify all consumers are updated
ast-grep --pattern 'worker.aggregate()'
# Search for any remaining *big.Int price handling that might need updating
rg -l 'big\.Int.*price'
Length of output: 108
Script:
#!/bin/bash
# Check the content of aggregator.go to understand the usage
cat x/oracle/keeper/aggregator/aggregator.go
# Search for any references to worker type or its methods
ast-grep --pattern 'type worker struct'
ast-grep --pattern 'func (w *worker)'
# Look for any price-related string handling that might be related
rg -l 'string.*price|price.*string'
Length of output: 8576
Script:
#!/bin/bash
# Check the worker implementation to understand the complete context
cat x/oracle/keeper/aggregator/worker.go
# Also check for any tests that might show the expected behavior
cat x/oracle/keeper/aggregator/helper_test.go
Length of output: 4514
108-108: Verify precision handling in string price representation
With the switch from *big.Int to string representation, we need to ensure that:
- The decimal precision is maintained as per the worker's
decimalfield - No precision loss occurs during price aggregation
✅ Verification successful
Let me gather more information about the aggregation implementation to verify the precision handling.
Let me gather more information about the aggregation implementation to verify how the decimal precision is handled.
Let me try one more search to find the aggregator implementation and price decimal handling.
Based on the code analysis, I can now provide a final response about the precision handling in the price representation.
Decimal precision is properly maintained in price aggregation
The verification shows that decimal precision is handled correctly:
-
The decimal precision is preserved through the aggregation process:
- Decimal value is obtained from token info configuration:
decimal: agc.params.GetTokenInfo(feederID).Decimal - Decimal validation is enforced through
CheckDecimalfunction that ensures reported prices match the configured decimal precision - The aggregator preserves the decimal value in the final price structure:
PriceTimeRound{ Price: finalPrice, Decimal: agc.params.GetTokenInfo(msg.FeederID).Decimal }
- Decimal value is obtained from token info configuration:
-
No precision loss occurs during aggregation:
- Price values are handled as strings and converted to
big.Intfor calculations - The aggregator maintains the full precision during calculations by using
big.Intarithmetic - The final aggregated price is converted back to string representation preserving the full precision
- Price values are handled as strings and converted to
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for price string format handling in the aggregator
ast-grep --pattern $'func \(.*aggregator\) aggregate\(\) string {
$$$
}'
# Search for decimal precision handling
rg -A 5 'decimal.*price'
Length of output: 9085
Script:
#!/bin/bash
# Look for the aggregator implementation
ast-grep --pattern $'func \(a \*aggregator\) aggregate\(\) string {
$$$
}'
# Search for price-related type definitions and conversions
rg -A 5 'type.*Price'
# Look for any string to decimal/number conversions in the oracle module
rg -A 5 'strconv\.' x/oracle/
Length of output: 96685
Script:
#!/bin/bash
# Look for the aggregator implementation
ast-grep --pattern 'func \(a \*aggregator\) aggregate\(\) string {
$$$
}'
# Look for decimal validation in the oracle module
rg -A 5 'decimal.*valid' x/oracle/
# Look for decimal conversion or handling in the oracle module
rg -A 5 'NewDecFromStr|NewDecFromInt' x/oracle/
Length of output: 1030
Script:
#!/bin/bash
# Search for aggregator implementation in worker.go
rg -A 10 'aggregate\(\) string' x/oracle/
# Look for decimal checks in the aggregator context
rg -A 5 'decimal' x/oracle/keeper/aggregator/
# Look for price validation in the aggregator
rg -A 5 'validate.*price|check.*price' x/oracle/keeper/aggregator/
Length of output: 7932
x/oracle/keeper/aggregator/calculator.go (2)
77-86: Verify copyBigInt implementation
The method uses copyBigInt for deep copying *big.Int values. Let's ensure this helper function is properly implemented.
✅ Verification successful
Implementation of copyBigInt is correct and safe
The copyBigInt implementation in x/oracle/keeper/aggregator/util.go is properly handling deep copying of *big.Int values by:
- Correctly handling nil input by returning nil
- Using
big.NewInt(0).Set(i)which creates a new big.Int and copies the value from the input, ensuring a true deep copy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for copyBigInt implementation
ast-grep --pattern 'func copyBigInt($_) *big.Int {
$$$
}'
Length of output: 385
13-13: Verify string format validation for price field
The change from *big.Int to string for price representation requires careful validation to ensure correct numerical format and precision.
✅ Verification successful
Based on the search results from both shell script executions, I can now generate the final response:
String price field requires validation before use
The change from *big.Int to string for price representation is validated through multiple layers:
- Price strings are validated for numerical format when converted using
sdkmath.NewIntFromStringin aggregator logic - Sanity checks ensure non-empty price values through
sanityCheckfunction - Price proposals go through validation rules defined in params
- Invalid price formats result in errors like
ErrPriceProposalIgnoredandErrInvalidMsg
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for price string validation logic
rg -A 5 'func.*[Vv]alidate.*[Pp]rice|func.*[Pp]arse.*[Pp]rice'
Length of output: 64
Script:
#!/bin/bash
# Let's search for any string-to-number conversion or validation logic for prices
rg -A 5 'price.*string|string.*price' -i
# Also search for any functions that might handle price validation
ast-grep --pattern 'func $_($$$) {
$$$
price
$$$
}'
Length of output: 53354
Script:
#!/bin/bash
# Let's search for validation or conversion logic specifically in the aggregator package
rg -A 5 'NewIntFromString.*[Pp]rice|[Vv]alidate.*[Pp]rice|[Cc]onvert.*[Pp]rice' x/oracle/keeper/aggregator/
# Also search for any price-related error handling
rg -A 3 'err.*[Pp]rice|[Pp]rice.*err' x/oracle/keeper/aggregator/
Length of output: 2903
x/oracle/keeper/aggregator/aggregator.go (3)
45-45: LGTM: Consistent type change for price representation.
The change to string for finalPrice while maintaining *big.Int for power calculations is appropriate.
224-224: LGTM: Consistent price field handling.
The price field assignment maintains consistency with the new string-based price representation while preserving the existing ordering and filtering logic.
12-12: Verify string-to-bigint conversion handling throughout the codebase.
The change from *big.Int to string for price fields allows for more flexible price representations (including non-standard formats) but requires careful handling of string-to-bigint conversions. Ensure all conversion points properly handle potential parsing errors.
Also applies to: 21-21
✅ Verification successful
Let me check the conversion handling in the aggregator implementation to ensure proper error handling.
String-to-bigint conversion handling is properly implemented
The code already implements proper error handling for string-to-bigint conversions:
- In both conversion points, the code checks the
okflag returned bySetString - When conversion fails (
!ok), it gracefully handles non-standard price formats by returning the original string price - This implementation allows for both numerical prices and non-standard price formats (NST) to coexist safely
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential string-to-bigint conversion points that might need error handling
rg "SetString|new\(big\.Int\).*String" --type go
Length of output: 542
Script:
#!/bin/bash
# Get the context around string-to-bigint conversions in the aggregator
rg "SetString|new\(big\.Int\).*String" x/oracle/keeper/aggregator/aggregator.go -B 3 -A 3
Length of output: 716
x/oracle/module.go (1)
161-163:
Ensure deterministic map iteration
The iteration over validatorPowers map could lead to non-deterministic behavior across nodes.
Consider sorting the validators before iteration:
+ // Get sorted validators for deterministic iteration
+ validators := make([]string, 0, len(validatorPowers))
+ for validator := range validatorPowers {
+ validators = append(validators, validator)
+ }
+ sort.Strings(validators)
- for validator := range validatorPowers {
+ for _, validator := range validators {
am.keeper.InitValidatorReportInfo(ctx, validator, ctx.BlockHeight())
}⛔ Skipped due to learnings
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/module.go:0-0
Timestamp: 2024-11-12T10:03:15.304Z
Learning: In `x/oracle/module.go`, iterating over `validatorPowers` without sorting is acceptable because the order does not affect consensus in this context.
x/oracle/keeper/aggregator/context.go (2)
207-208: LGTM: Good improvements to error handling
The changes properly propagate the context and use structured error wrapping, which improves error handling and debugging capabilities.
Line range hint 124-146: Consider revising the error handling strategy in check mode
The current implementation suppresses errors during transaction simulation (isCheckMode), which could lead to inconsistent behavior between simulation and actual execution. This might cause transactions to pass simulation but fail during actual execution.
Consider one of these alternatives:
- Return a specific simulation-only error type that can be handled differently by the caller
- Add logging in check mode to help diagnose potential issues
Let's verify the impact of this change on transaction simulation:
x/oracle/keeper/native_token.go (1)
423-428: LGTM: Type conversion is safe
The helper functions have been correctly updated to use the new NSTAssetID type. The type conversion is safe as it's just a string type alias, and all network values are properly initialized in the maps.
x/oracle/types/params.go (3)
Line range hint 1-500: LGTM with validations
The extensive validation logic in the Params.Validate() method provides good safeguards for the new configurations. The method includes comprehensive checks for:
- Token parameters
- TokenFeeder parameters
- Cross-validation between different components
Line range hint 67-115: Verify RuleID configuration consistency
Both ETH and NSTETH tokens use RuleID: 1, which implies they share the same price source rules. Verify this is intentional and the rules are appropriate for both tokens.
✅ Verification successful
Based on the search results, I can now provide a final response as we have sufficient information about the RuleSource configuration:
RuleID 1 configuration is appropriate for both ETH and NSTETH tokens
The RuleSource with ID 1 is configured with SourceIDs: []uint64{0}, where according to the code comments, setting the first source ID to 0 means "all valid sources" will be used. This is an intentional and appropriate configuration for both tokens as:
- It ensures maximum price accuracy by considering all valid price sources
- It provides consistency in price sourcing for related assets (ETH and its staked variant NSTETH)
- The configuration aligns with the protocol's design for price feed validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RuleSource configurations
ast-grep --pattern 'type RuleSource struct {
$$$
}'
# Search for documentation about rule configurations
rg "RuleSource" -A 5 -B 5
Length of output: 18115
109-115: Verify StartBaseBlock configuration
The StartBaseBlock (10) is significantly lower than the ETH token's configuration (1000000). This large difference might indicate:
- A potential configuration error
- Different requirements for NSTETH price updates
✅ Verification successful
Based on the search results, I can now provide a final response as there's sufficient context about the StartBaseBlock configurations and their purpose:
Different StartBaseBlock values are intentional and correct
The different StartBaseBlock values between ETH (1000000) and NSTETH (10) tokens are intentional:
- The code shows consistent patterns of different StartBaseBlock values being used for different tokens:
- In genesis data: ETH token uses 10, NSTETH uses 7
- In test cases: Various values (1, 10, 1000000) are used
- The StartBaseBlock parameter represents when price updates begin for a token, and the documentation states: "include, first block which start_round_id can be settled is at least start_base_block+1"
- The validation logic ensures StartBaseBlock values are properly validated against overlaps and continuity between token feeders
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other TokenFeeder configurations to verify StartBaseBlock patterns
ast-grep --pattern 'TokenID: $_,
RuleID: $_,
StartRoundID: $_,
StartBaseBlock: $_,
Interval: $_'
# Search for documentation or comments about StartBaseBlock configuration
rg "StartBaseBlock" -A 3 -B 3
Length of output: 26447
testutil/network/network.go (3)
84-91: LGTM: Config struct changes are well-documented
The new fields are properly documented and follow consistent naming conventions.
495-505: 🛠️ Refactor suggestion
Move hardcoded values to configuration
The gateway setup has two issues:
- Hardcoded gateway address reduces flexibility
- Fixed commission rate should be configurable
Consider moving these to the Config struct for better maintainability and testing.
+type Config struct {
+ // ... existing fields ...
+ GatewayAddress string
+ CommissionRate string
+}
+func DefaultConfig() Config {
+ return Config{
+ // ... existing initialization ...
+ GatewayAddress: "exo18cggcpvwspnd5c6ny8wrqxpffj5zmhklprtnph",
+ CommissionRate: "0.5",
+ }
}
-commissionRate, _ := sdkmath.LegacyNewDecFromStr("0.5")
-gateWayAddressStr := "exo18cggcpvwspnd5c6ny8wrqxpffj5zmhklprtnph"
+commissionRate, err := sdkmath.LegacyNewDecFromStr(cfg.CommissionRate)
+if err != nil {
+ return nil, fmt.Errorf("invalid commission rate: %v", err)
+}
+gateWayAddr, err := sdk.AccAddressFromBech32(cfg.GatewayAddress)Likely invalid or redundant comment.
380-394: 🛠️ Refactor suggestion
Enhance key generation security
While switching to Ed25519 is good, consider these security improvements:
- Add entropy parameter to mnemonic generation
- Verify the key derivation path is appropriate for Ed25519
-_, mnemonic, err := kb.NewMnemonic(fmt.Sprintf("valconskey%d", i), keyring.English, sdk.GetConfig().GetBech32AccountPubPrefix(), keyring.DefaultBIP39Passphrase, algo)
+entropy, err := bip39.NewEntropy(256)
+if err != nil {
+ return nil, err
+}
+_, mnemonic, err := kb.NewMnemonic(
+ fmt.Sprintf("valconskey%d", i),
+ keyring.English,
+ sdk.GetConfig().GetBech32AccountPubPrefix(),
+ keyring.DefaultBIP39Passphrase,
+ algo,
+ entropy,
+)Likely invalid or redundant comment.
x/delegation/keeper/update_native_restaking_balance.go (1)
48-49: Approved: Logic adjustment for pendingSlashAmount
The adjustment to set pendingSlashAmount to zero when it is not positive ensures correct handling of slashing amounts and prevents potential negative values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
x/oracle/keeper/aggregator/calculator_test.go (1)
57-60: LGTM! Consider enhancing test readabilityThe string representation changes are correct and maintain the test's ability to handle different source orderings.
Consider extracting the expected values into named constants to improve test maintainability:
+ const ( + expectedPrice1 = "12" + expectedPrice2 = "15" + ) So(confirmed[i].detID, ShouldEqual, "2") - So(confirmed[i].price, ShouldResemble, "12") + So(confirmed[i].price, ShouldResemble, expectedPrice1) So(confirmed[1-i].detID, ShouldEqual, "3") - So(confirmed[1-i].price, ShouldResemble, "15") + So(confirmed[1-i].price, ShouldResemble, expectedPrice2)x/oracle/keeper/aggregator/aggregator_test.go (3)
18-18: LGTM! Consider enhancing test readability.The initial price validation checks are correct, using string representation as expected.
Consider adding comments to clarify the expected empty state:
-So(report.prices[1].price, ShouldEqual, "") +// Verify price is initially empty before confirmation +So(report.prices[1].price, ShouldEqual, "")Also applies to: 22-22, 26-27
33-33: Consider adding boundary test cases.While the current test cases validate the basic functionality, consider adding tests for:
- Edge cases (very large numbers, zero)
- Invalid price formats
- Negative prices (if applicable)
Example addition:
{ sourceID: 1, detID: "10", price: "999999999999999999", // Test large numbers timestamp: "-", },Also applies to: 39-39, 45-45, 48-48, 51-51
58-61: Document the price aggregation formula.While the test validates the final prices correctly, it would be beneficial to document how "15" is derived from the input prices.
Add a comment block explaining the calculation:
+// Price aggregation formula: +// v1: average of source1 (10) and source2 (20) = 15 +// v2: only source1 (10) = 10 +// v3: only source2 (20) = 20 +// finalPrice: median of validator prices (10, 15, 20) = 15 So(a.getReport("v1").price, ShouldEqual, "15")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
testutil/network/genesis_data.go(1 hunks)x/oracle/keeper/aggregator/aggregator_test.go(1 hunks)x/oracle/keeper/aggregator/calculator_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- testutil/network/genesis_data.go
🔇 Additional comments (4)
x/oracle/keeper/aggregator/calculator_test.go (3)
39-39: LGTM! String representation matches test scenario
The assertion correctly validates the expected price value of "10" for detid=1 consensus case.
66-66: LGTM! Consistent with single source consensus test
The string representation correctly validates the expected price for the single source consensus scenario.
34-34: LGTM! Verify string conversion handling
The change from big.Int to string representation is consistent with the codebase-wide updates.
✅ Verification successful
Based on the search results, I can see that there's been a consistent transition from big.Int to string representation for prices across the codebase. The test assertions are correctly using string comparisons, and the underlying types (PriceTimeDetID, PriceTimeRound, etc.) all use string representation for prices. This change is part of a broader, intentional modification to handle prices as strings throughout the system.
String price representation is the correct approach
The change from big.Int to string for price assertions is consistent with the codebase's design where prices are stored and handled as strings. This is evidenced by:
- The
PriceTimeDetIDandPriceTimeRoundstructs use string for price fields - All price-related test assertions across the codebase use string comparisons
- The aggregator and calculator components work with string prices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent price type usage across the codebase
ast-grep --pattern 'price.*big.Int'
Length of output: 37
Script:
#!/bin/bash
# Search for price-related assertions in test files
rg -A 2 'price.*ShouldResemble' --type go
# Search for price type declarations and usages
rg -A 2 'type.*Price' --type go
rg -A 2 'price.*string' --type go
Length of output: 63317
x/oracle/keeper/aggregator/aggregator_test.go (1)
Line range hint 1-67: Verify consistent string representation across related files.
The transition to string representation for prices is well implemented in this test file.
Let's verify consistency across related files:
✅ Verification successful
Let me gather more information to verify the consistency of string representation across the codebase.
Based on the comprehensive search results, I can now generate the final response:
String representation for prices is consistently implemented across the codebase
The verification shows:
- All price-related structs and fields use string type for price values
- Price comparisons and validations consistently handle string representations
- The only
big.Intusage is for internal calculations incalculator.goandaggregator.go, where prices are properly converted to/from strings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining big.Int price fields or numerical price comparisons
rg -t go "price.*big\.Int|price.*ShouldEqual.*[0-9]+" x/oracle/
# Check for consistent string price handling
ast-grep --pattern 'price.*ShouldEqual.*"[0-9]+"' x/oracle/
Length of output: 1107
Script:
#!/bin/bash
# Check for any price-related type definitions and struct fields
rg -t go "type.*[Pp]rice|price.*string|price.*[Ii]nt" x/oracle/
# Check for price-related function signatures and return types
ast-grep --pattern 'func $_($$$) $_Price' x/oracle/
# Look for price comparison or validation logic
rg -t go "compare.*[Pp]rice|validate.*[Pp]rice" x/oracle/
Length of output: 24122
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
testutil/network/genesis_data.go (1)
49-54: Document empty genesis states.The empty genesis states for Operator and Delegation modules need explanation. The commented-out default genesis initialization suggests incomplete implementation.
Add comments explaining why these genesis states are empty and when they should be populated.
testutil/network/tx_precompile.go (3)
21-22: Consider making gas limit configurableThe hardcoded gas limit of 500000 might not be suitable for all precompile transactions. Consider making it configurable or using gas estimation.
-const ( - privateKey = "D196DCA836F8AC2FFF45B3C9F0113825CCBB33FA1B39737B948503B263ED75AE" // gitleaks:allow - gasLimit = uint64(500000) +var ( + privateKey = os.Getenv("EXOCORE_TEST_PRIVATE_KEY") // Default to a test key if not set + gasLimit = uint64(500000) // Default gas limit ) +// SetGasLimit allows configuring the gas limit for precompile transactions +func SetGasLimit(limit uint64) { + gasLimit = limit +}
36-39: Document precompile addressesThe precompile addresses are hardcoded without documentation explaining their significance or how they were derived. Consider adding documentation about these addresses and their purpose.
var ( + // precompileAddresses maps precompile names to their designated addresses in the Exocore network + // 0x804: Assets precompile for managing network assets + // 0x805: Delegation precompile for managing validator delegations precompileAddresses = map[precompile]common.Address{ ASSETS: common.HexToAddress("0x0000000000000000000000000000000000000804"), DELEGATION: common.HexToAddress("0x0000000000000000000000000000000000000805"), }
124-131: Enhance parseABI error handlingThe
parseABIfunction could provide more context in error messages to help diagnose ABI parsing issues.func parseABI(abiPath string) (abi.ABI, error) { f, err := os.Open(abiPath) if err != nil { - return abi.ABI{}, err + return abi.ABI{}, fmt.Errorf("failed to open ABI file %s: %w", abiPath, err) } defer f.Close() - return abi.JSON(f) + abi, err := abi.JSON(f) + if err != nil { + return abi, fmt.Errorf("failed to parse ABI JSON from %s: %w", abiPath, err) + } + return abi, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
tests/e2e/bank/tx.go(1 hunks)testutil/network/genesis_data.go(1 hunks)testutil/network/tx_precompile.go(1 hunks)x/oracle/keeper/native_token.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/bank/tx.go
- x/oracle/keeper/native_token.go
🧰 Additional context used
📓 Learnings (1)
testutil/network/genesis_data.go (2)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore#192
File: x/assets/keeper/client_chain_asset.go:69-79
Timestamp: 2024-11-12T10:03:15.304Z
Learning: When suggesting input validation, be aware that there may be duplicated code around validations that can be refactored. Consider proposing the creation of `ClientChainInfo.Validate` and `StakingAssetInfo.Validate` functions, and suggest calling them in the relevant functions as well as in `genesis.Validate`.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: testutil/network/util.go:292-349
Timestamp: 2024-11-15T07:01:04.078Z
Learning: In the Go code for Exocore, specifically in the `testutil/network/util.go` file, functions like `NewGenStateAssets` are expected to modify the global variable `DefaultGenStateAssets`. Modifying global variables in these functions is acceptable and expected behavior in this context.
🔇 Additional comments (4)
testutil/network/genesis_data.go (2)
15-24:
Consider using dedicated test addresses instead of production ones.
While the constants are well-documented, using production addresses (USDT, ETH) in tests could lead to confusion and potential security risks if test configurations accidentally make it to production.
Consider using clearly marked test addresses:
- NativeAssetAddress = "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee"
- ETHAssetAddress = "0xdac17f958d2ee523a2206206994597c13d831ec7"
+ NativeAssetAddress = "0xTEST000000000000000000000000000000000001"
+ ETHAssetAddress = "0xTEST000000000000000000000000000000000002"60-85:
Document magic numbers and add bounds checking.
The init function uses magic numbers and assumes array indices exist without validation.
Consider these improvements:
- Document the significance of magic numbers:
+const (
+ // ETHTokenFeederStartBlock is the initial block for ETH price feeds
+ ETHTokenFeederStartBlock = 10
+ // NSTTokenFeederStartBlock is the initial block for NST price feeds
+ NSTTokenFeederStartBlock = 7
+)- Add bounds checking:
func init() {
+ if len(DefaultGenStateOracle.Params.Tokens) < 2 {
+ panic("insufficient tokens in oracle genesis state")
+ }
+ if len(DefaultGenStateOracle.Params.TokenFeeders) < 2 {
+ panic("insufficient token feeders in oracle genesis state")
+ }Likely invalid or redundant comment.
testutil/network/tx_precompile.go (2)
43-58: 🛠️ Refactor suggestion
Improve initialization error handling
The init function uses panic for error handling, which is not ideal for a test utility. Consider making the initialization explicit and returning errors.
-func init() {
+// InitializePrecompiles initializes the precompile ABIs
+func InitializePrecompiles() error {
_, currentFile, _, ok := runtime.Caller(0)
if !ok {
- fmt.Println("Failed to get current file path")
- panic("Failed to get current file path")
+ return fmt.Errorf("failed to get current file path")
}
basePath := filepath.Dir(currentFile)
var err error
for precompileName, precompilePath := range precompilePaths {
p := filepath.Join(basePath, precompilePath)
abis[precompileName], err = parseABI(p)
if err != nil {
- panic(fmt.Sprintf("Failed to parse abi from path:%s\r\n", p))
+ return fmt.Errorf("failed to parse ABI from path %s: %w", p, err)
}
}
+ return nil
}Likely invalid or redundant comment.
62-64: 🛠️ Refactor suggestion
Add context timeout and improve context handling
Using context.Background() without a timeout could lead to hanging transactions. Consider adding a timeout and accepting a context parameter.
-func (n Network) SendPrecompileTx(preCompileName precompile, methodName string, args ...interface{}) error {
- ctx := context.Background()
+func (n Network) SendPrecompileTx(ctx context.Context, preCompileName precompile, methodName string, args ...interface{}) error {
+ if ctx == nil {
+ var cancel context.CancelFunc
+ ctx, cancel = context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
+ }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/oracle/keeper/aggregator/aggregator.go (2)
27-41: Consider adding error logging for failed price conversions.The price aggregation logic correctly handles both numeric and non-numeric prices. However, it would be helpful to log when price conversion fails to aid in debugging.
priceInt, ok := new(big.Int).SetString(p.price, 10) // price is not a number (NST), we will just return instead of calculation if !ok { + // Log conversion failure for debugging + log.Printf("Failed to convert price %s to big.Int for aggregation", p.price) return p.price }
186-192: Consider enhancing error context for non-numeric prices.While the handling of non-numeric prices is correct, adding more context about which validator's price caused the conversion failure would be helpful for debugging.
priceInt, ok := new(big.Int).SetString(validatorReport.aggregate(), 10) if !ok { + // Log which validator's price caused the conversion failure + log.Printf("Non-numeric price from validator %s: %s", validatorReport.validator, validatorReport.aggregate()) // price is not number, we just return the price when power exceeds threshold agg.finalPrice = validatorReport.aggregate() return agg.finalPrice }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
x/oracle/keeper/aggregator/aggregator.go(7 hunks)x/oracle/keeper/aggregator/calculator.go(4 hunks)x/oracle/keeper/aggregator/util.go(1 hunks)x/oracle/keeper/native_token.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- x/oracle/keeper/aggregator/calculator.go
- x/oracle/keeper/aggregator/util.go
- x/oracle/keeper/native_token.go
🧰 Additional context used
📓 Learnings (1)
x/oracle/keeper/aggregator/aggregator.go (2)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: x/oracle/keeper/aggregator/aggregator.go:107-113
Timestamp: 2024-11-27T06:16:50.976Z
Learning: For non-deterministic sources (NS), string values for prices are acceptable without additional validation before storage.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: x/oracle/keeper/aggregator/context.go:187-191
Timestamp: 2024-11-27T06:19:17.106Z
Learning: In the `x/oracle/keeper/aggregator/context.go` file, we accept 'nan' strings for NST scenarios and do not need to add price validation in this context.
🔇 Additional comments (5)
x/oracle/keeper/aggregator/aggregator.go (5)
12-12: LGTM! Type changes align with the new string-based price representation.
The conversion from *big.Int to string for price fields is consistent across the structs.
Also applies to: 21-21
45-45: LGTM! Consistent type change for finalPrice.
The change to string type for finalPrice maintains consistency with the new price representation model.
107-113: LGTM! Non-deterministic source price handling is correct.
The direct string assignment for NS prices aligns with the established requirements, as confirmed by previous learnings.
Also applies to: 124-125
148-148: LGTM! Price validation check is appropriate.
The length check for string prices is the correct approach for validating completed IVA prices.
224-224: LGTM! Return type is consistent.
The price field type in the return structure correctly uses the string type.
Description
This PR supports testutil/network to work properly with exocorenetwork.
It starts a exocore network with n in process nodes with n validators. We can write UT to do e2e test by interact with this in-process network.
And this PR has a bug fix for NST balance change fail caused by Inappropriate assetID configure.
Notable Changes
testutil/netowrk/: e2e frameworkdtests/e2e/: e2e test casesbank/oracle/: LST price update, NST balance change, slashingx/oracle/: oracle NST blanceupdate bug fixCloses #XXX
Summary by CodeRabbit
Release Notes
New Features
Improvements
big.Intto string types, simplifying the codebase.Bug Fixes
Chores
go.modfile for improved compatibility and functionality.These updates enhance user experience, improve system reliability, and ensure robust testing of critical functionalities.