feat: refactor consumers config #55
Conversation
freak12techno
changed the title
WalkthroughThe changes encompass significant updates across various files within the codebase, including structural modifications to configuration, fetching, and RPC handling. Notably, new interfaces, structs, and methods are introduced for handling and validating configurations, querying, and managing consumer chains. The transformation enhances functionality and improves the flexibility of configurations and RPC interactions, notably shifting from Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration 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: 5
Outside diff range and nitpick comments (5)
pkg/config/query.go (1)
3-11: TheQueriestype and itsEnabledmethod are correctly implemented. Consider adding documentation for the method to explain the default behavior of enabling all queries unless explicitly disabled.pkg/fetchers/validators.go (1)
Line range hint
54-75: Ensure proper handling of goroutines and synchronization. The use ofRPC.RPCmight be confusing and could benefit from renaming for clarity.Consider renaming
RPC.RPCto something more descriptive to avoid confusion, such asBaseRPC.pkg/fetchers/rewards.go (1)
Line range hint
59-101: Concurrency is handled well withsync.WaitGroupandsync.Mutex. However, consider adding error handling for therpclookup.- rpc, _ := q.RPCs[chain.Name] + rpc, exists := q.RPCs[chain.Name] + if !exists { + q.Logger.Error().Str("chain", chain.Name).Msg("RPC configuration not found") + continue + }pkg/fetchers/self_delegation.go (1)
Line range hint
60-102: Concurrency is handled well withsync.WaitGroupandsync.Mutex. However, consider adding error handling for therpclookup.- rpc, _ := q.RPCs[chain.Name] + rpc, exists := q.RPCs[chain.Name] + if !exists { + q.Logger.Error().Str("chain", chain.Name).Msg("RPC configuration not found") + continue + }pkg/tendermint/rpc.go (1)
[!TIP]
Codebase VerificationThe verification scripts did not find any unit tests in
pkg/tendermint/rpc.goorpkg/utils/utils.go, which are the files where the parsing logic is present. This indicates a potential lack of test coverage for the parsing logic.
- Please ensure that the parsing logic for the threshold value is robust and handles potential edge cases.
- Add unit tests to cover the parsing logic in
pkg/tendermint/rpc.goandpkg/utils/utils.go.Analysis chain
Line range hint
410-422:GetConsumerSoftOutOutThresholdmethod uses theEnabledcheck, which is consistent with other methods. Ensure that the parsing logic for the threshold value is robust and handles potential edge cases.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the robustness of parsing logic for threshold values. # Test: Search for unit tests covering the parsing logic. rg --type python $'strconv.ParseFloat'Length of output: 68
Script:
#!/bin/bash # Description: Verify the robustness of parsing logic for threshold values. # Step 1: Search for the parsing logic in Go files rg --type go 'ParseFloat|ParseInt|Atof|Atoi' # Step 2: Search for unit tests covering the parsing logic rg --type go 'ParseFloat|ParseInt|Atof|Atoi' --files-with-matches | xargs -I {} sh -c 'rg -l "func Test" {}'Length of output: 417
Script:
#!/bin/bash # Description: Verify the robustness of parsing logic for threshold values. # Step 1: Search for unit tests in the identified files rg --type go 'func Test' pkg/tendermint/rpc.go pkg/utils/utils.goLength of output: 67
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (29)
- .golangci.yml (2 hunks)
- pkg/app.go (2 hunks)
- pkg/config/chain.go (1 hunks)
- pkg/config/chain_info.go (1 hunks)
- pkg/config/config.go (2 hunks)
- pkg/config/consumer_chain.go (1 hunks)
- pkg/config/denom_info.go (1 hunks)
- pkg/config/query.go (1 hunks)
- pkg/config/tracing.go (1 hunks)
- pkg/fetchers/balance.go (3 hunks)
- pkg/fetchers/commission.go (4 hunks)
- pkg/fetchers/delegations.go (4 hunks)
- pkg/fetchers/node_info.go (3 hunks)
- pkg/fetchers/rewards.go (4 hunks)
- pkg/fetchers/self_delegation.go (4 hunks)
- pkg/fetchers/signing_info.go (4 hunks)
- pkg/fetchers/slashing_params.go (3 hunks)
- pkg/fetchers/soft_opt_out_threshold.go (3 hunks)
- pkg/fetchers/staking_params.go (4 hunks)
- pkg/fetchers/unbonds.go (4 hunks)
- pkg/fetchers/validators.go (4 hunks)
- pkg/generators/active_set_tokens.go (1 hunks)
- pkg/generators/denom_coefficient.go (2 hunks)
- pkg/generators/is_consumer.go (2 hunks)
- pkg/generators/single_validator_info.go (3 hunks)
- pkg/generators/validator_rank.go (1 hunks)
- pkg/tendermint/rpc.go (25 hunks)
- pkg/tendermint/rpc_with_consumers.go (1 hunks)
- pkg/types/tendermint.go (1 hunks)
Files not reviewed due to errors (6)
- pkg/config/tracing.go (no review received)
- pkg/fetchers/node_info.go (no review received)
- pkg/fetchers/delegations.go (no review received)
- pkg/generators/single_validator_info.go (no review received)
- pkg/fetchers/signing_info.go (no review received)
- pkg/app.go (no review received)
Additional comments not posted (59)
pkg/config/chain_info.go (1)
3-7: InterfaceChainInfois well-defined with essential methods for chain operations..golangci.yml (1)
26-26: The linter settings have been updated to disable a significant number of linters. Ensure that the disabling of these linters is justified and documented, especially for critical ones likegosecanderrorlint. This could impact the code quality and security assessments.pkg/generators/is_consumer.go (2)
12-12: Using a slice of pointers inIsConsumerGeneratoris a good practice for memory efficiency and ensuring data consistency across the application.
Line range hint
15-37: TheGeneratemethod effectively utilizes Prometheus gauges and labels to monitor whether chains are consumers, showcasing good use of external libraries for monitoring.pkg/config/denom_info.go (2)
9-16: TheDenomInfostruct is well-structured with sensible defaults and serialization tags, ensuring that the denomination data is robustly managed.
30-37: TheDisplayWarningsmethod proactively logs configuration issues using structured logging, which is an excellent practice for maintaining system health.pkg/config/consumer_chain.go (5)
8-18: TheConsumerChainstruct is well-defined, effectively handling complex configurations with appropriate data types and serialization tags.
32-51: TheValidatemethod inConsumerChainthoroughly checks all necessary fields and validatesDenomInfowithin a loop, ensuring the integrity of the consumer chain configuration.
20-22: TheGetQueriesmethod correctly provides access to theQueriesfield, following good encapsulation practices.
24-26: TheGetHostmethod correctly encapsulates access to theLCDEndpointfield.
28-30: TheGetNamemethod correctly provides access to theNamefield, adhering to good encapsulation practices.pkg/fetchers/validators.go (2)
18-18: RefactoredRPCsfield to useRPCWithConsumersaligns with the new architecture to handle consumer chains.
29-29: Correctly updated the constructor to acceptRPCWithConsumerswhich matches the structural change in theValidatorsFetcherstruct.pkg/fetchers/staking_params.go (3)
18-18: RefactoredRPCsfield to useRPCWithConsumersaligns with the new architecture to handle consumer chains.
29-29: Correctly updated the constructor to acceptRPCWithConsumers, ensuring consistency with the structural change in theStakingParamsFetcherstruct.
Line range hint
55-81: Good addition of comments explaining the context of fetching staking parameters only for provider chains. Proper error handling and synchronization in the goroutine.pkg/generators/denom_coefficient.go (1)
Line range hint
12-66: The refactoring to use pointers inChainsis consistent with changes in other parts of the codebase. The iteration over consumer chains and handling of denominations are well-implemented. Ensure that theDenomCoefficientfield exists in theDenomInfostruct.Verification successful
The
DenomCoefficientfield exists in theDenomInfostruct as expected.
pkg/config/denom_info.go:
DenomCoefficient int64default:"1000000" toml:"denom-coefficient"`Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of `DenomCoefficient` field in the `DenomInfo` struct. # Test: Search for the `DenomCoefficient` field definition. Expect: Definition found. rg --type go $'type DenomInfo struct {.*DenomCoefficient.*}'Length of output: 211
Script:
#!/bin/bash # Description: Verify the existence of `DenomCoefficient` field in the `DenomInfo` struct. # Test: Search for the `DenomCoefficient` field definition. Expect: Definition found. rg --type go 'type DenomInfo struct {[^}]*DenomCoefficient[^}]*}'Length of output: 223
Script:
#!/bin/bash # Description: Verify the existence of `DenomCoefficient` field in the `DenomInfo` struct. # Test: Search for the `DenomInfo` struct definition and check for the `DenomCoefficient` field within it. rg --type go 'type DenomInfo struct' -A 10 | rg 'DenomCoefficient'Length of output: 174
pkg/config/config.go (2)
30-30: Update the default value ofListenAddressto":9560"to accommodate new port requirements.
32-32: Switched from a slice ofChainto a slice of pointers toChainto potentially reduce memory usage and enhance performance when manipulating large configurations.pkg/fetchers/soft_opt_out_threshold.go (3)
18-18: UpdatedRPCsto useRPCWithConsumersreflecting the new structure to handle multiple consumers for each chain.
29-29: Constructor now acceptsmap[string]*tendermint.RPCWithConsumersto initialize the fetcher with the updated RPC structure.
53-80: Refactored the fetch logic to handle multiple consumer chains per RPC. This change allows fetching thresholds for each consumer chain separately, improving data granularity and potentially enhancing error isolation in multi-consumer environments.pkg/fetchers/commission.go (3)
18-18: UpdatedRPCsto useRPCWithConsumers, aligning with the new architecture to support multiple consumer chains.
29-29: Constructor updated to acceptRPCWithConsumers, ensuring compatibility with the new RPC handling mechanism.
Line range hint
47-86: Enhanced the logic to fetch commission data for each validator across all chains, now considering the updated RPC structure. The new implementation is designed to handle multiple consumers per chain, which is crucial for accurate data fetching in environments with complex chain configurations.pkg/fetchers/unbonds.go (3)
19-19: UpdatedRPCsto useRPCWithConsumers, aligning with the new architecture to support multiple consumer chains.
30-30: Constructor updated to acceptRPCWithConsumers, ensuring compatibility with the new RPC handling mechanism.
Line range hint
60-88: Refactored the logic to fetch unbonding information for each validator across all chains, considering the updated RPC structure. This implementation is crucial for environments where chains have multiple consumers, ensuring accurate and isolated data retrieval.pkg/generators/active_set_tokens.go (3)
16-16: The change to use pointers forChainsinActiveSetTokensGeneratoris appropriate for efficient memory management and consistent with the broader refactoring strategy observed in other parts of the system.
19-19: Correct implementation of the constructor forActiveSetTokensGeneratorusing pointers, ensuring memory efficiency and consistency with the rest of the application's architecture.
19-19: TheGeneratefunction is well-implemented, handling multiple edge cases and integrating various components effectively to produce the desired metrics.pkg/fetchers/slashing_params.go (3)
18-18: Updating theRPCsfield to useRPCWithConsumersaligns with the refactor goal to enhance concurrent processing capabilities and manage multiple consumer chains more effectively.
35-35: Proper initialization ofSlashingParamsFetcherwithRPCWithConsumers, ensuring consistency with the new system architecture.
49-67: TheFetchmethod is robust, effectively managing concurrency and error handling while fetching slashing parameters across multiple chains and their consumers.pkg/fetchers/rewards.go (3)
19-19: Refactor to useRPCWithConsumersaligns with the updated architecture.
30-30: Constructor correctly initializes theRewardsFetcherwith the newRPCWithConsumerstype.
47-49: Initialization ofallRewardsis clear and correctly scoped within the fetch loop.pkg/fetchers/self_delegation.go (3)
19-19: Refactor to useRPCWithConsumersaligns with the updated architecture.
30-30: Constructor correctly initializes theSelfDelegationFetcherwith the newRPCWithConsumerstype.
48-51: Initialization ofallSelfDelegationsis clear and correctly scoped within the fetch loop.pkg/generators/validator_rank.go (2)
18-18: Use of pointers forChainsinValidatorRankGeneratoraligns with the updated data structure.
23-23: Constructor correctly initializes theValidatorRankGeneratorwith the new pointer-basedChains.pkg/fetchers/balance.go (3)
19-19: Refactor to useRPCWithConsumersaligns with the updated architecture.
36-36: Constructor correctly initializes theBalanceFetcherwith the newRPCWithConsumerstype.
50-57: Initialization ofallBalancesis clear and correctly scoped within the fetch loop. Handling of consumer chains is also correctly implemented.pkg/types/tendermint.go (1)
123-126: The addition ofAssignedKeyResponsestruct aligns well with existing response structures and appears to be correctly integrated.pkg/tendermint/rpc.go (13)
21-27: LGTM! The updatedRPCstruct now includesChainQueries, which supports the new configurable query enabling feature.
33-46: Improved constructor functionNewRPCusing theChainInfointerface. This aligns with the new architecture, allowing more flexibility and encapsulation of chain-related information.
Line range hint
127-143: The methodGetSingleDelegationcorrectly checks the query enablement before proceeding. This pattern is consistent and should be maintained across all similar methods.
Line range hint
166-176: Consistent use of theEnabledcheck inGetAllValidators. This is a critical pattern ensuring that only enabled queries are processed.
Line range hint
196-209: TheGetValidatorCommissionmethod reflects consistent application of the new query enabling system. It's crucial that all such methods follow this pattern to ensure system integrity.
Line range hint
274-287: The methodGetWalletBalancecorrectly uses the newChainQueriesstructure to check if the query is enabled, which is a good practice.
Line range hint
351-362: The methodGetSigningInfocorrectly uses theEnabledcheck. This consistency is key to the new configuration's effectiveness.
Line range hint
381-391:GetSlashingParamsmethod uses theEnabledcheck effectively. Consistency in these checks across the system is crucial.
Line range hint
448-458:GetStakingParamsmethod consistently uses theEnabledcheck. This method's implementation aligns with the system's architectural changes.
Line range hint
477-487:GetNodeInfomethod's implementation is consistent with the new system's architecture, using theEnabledcheck effectively.
Line range hint
233-249: Use ofEnabledinGetDelegatorRewardsmaintains consistency. As with other similar methods, ensure this logic is backed by robust tests.
Line range hint
57-70: Refactor inGetDelegationsCountmethod to useChainQueries.Enabledis a good use of the new configuration capabilities. However, ensure that theEnabledmethod is thoroughly tested to handle all expected inputs correctly.
Line range hint
92-105: Similar to the previous method,GetUnbondsCountalso uses theEnabledmethod effectively. Ensure comprehensive test coverage for these query checks.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- pkg/config/denom_info.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- pkg/config/denom_info.go
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- pkg/tendermint/rpc.go (25 hunks)
Additional comments not posted (3)
pkg/tendermint/rpc.go (3)
21-27: Refactoring of the RPC struct to accommodate the new configuration structure looks clean and well-integrated.
33-47: The constructor functionNewRPCis updated to use theChainInfointerface, which is a good practice as it decouples the RPC functionality from the concreteChainstruct and adheres to dependency inversion principles.
Line range hint
58-71: Ensure allChainQueries.Enabledchecks are correctly implemented for the respective RPC calls. This is a critical part of the logic to prevent unnecessary API calls when certain features are disabled.Also applies to: 93-106, 128-144, 167-177, 197-210, 234-250, 275-288, 308-332, 352-363, 382-392, 411-423, 449-459, 478-488
Verification successful
The extracted lines confirm that each
ChainQueries.Enabledcheck is correctly placed before its respective RPC call to prevent unnecessary API calls when certain features are disabled.
- Lines 55-61: The check for
"delegations"is correctly implemented.- Lines 93-106, 128-144, 167-177, 197-210, 234-250, 275-288, 308-332, 352-363, 382-392, 411-423, 449-459, 478-488: All other checks for various features are also correctly implemented.
The review comment is verified as correct.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct implementation of ChainQueries.Enabled checks. # Test: Search for "ChainQueries.Enabled" usage and ensure it matches expected logic. rg --type go 'ChainQueries.Enabled'Length of output: 924
Script:
#!/bin/bash # Extract relevant lines where ChainQueries.Enabled is used in pkg/tendermint/rpc.go rg --type go --line-number 'ChainQueries.Enabled' pkg/tendermint/rpc.go | cut -d: -f1 | xargs -I {} sed -n '{}p' pkg/tendermint/rpc.goLength of output: 739
Script:
#!/bin/bash # Extract lines around each ChainQueries.Enabled check in pkg/tendermint/rpc.go to review the context rg --type go --line-number 'ChainQueries.Enabled' pkg/tendermint/rpc.go | cut -d: -f1 | xargs -I {} sed -n '{}-2,{}+4p' pkg/tendermint/rpc.goLength of output: 819
Script:
#!/bin/bash # Extract lines around each ChainQueries.Enabled check in pkg/tendermint/rpc.go to review the context rg --type go --line-number 'ChainQueries.Enabled' pkg/tendermint/rpc.go | cut -d: -f1 | xargs -I {} sed -n -e '{}-2,{}+4p' pkg/tendermint/rpc.goLength of output: 822
Script:
#!/bin/bash # Extract lines around each ChainQueries.Enabled check in pkg/tendermint/rpc.go to review the context rg --type go --line-number 'ChainQueries.Enabled' pkg/tendermint/rpc.go | cut -d: -f1 | xargs -I {} sh -c 'sed -n "$(({}-2)),$(({}+4))p" pkg/tendermint/rpc.go'Length of output: 2656
Summary by CodeRabbit
New Features
GetConsumerAssignedKeymethod to the RPC struct for key management.Bug Fixes
tendermint.RPCWithConsumers.Enhancements
RPCstruct to includeChainName,ChainHost, andChainQueriesfor better context management.