feat: refactor denoms #64
Conversation
WalkthroughThis update adds significant enhancements to the application by introducing support for handling currency denominations, improving validation logic, and enriching Prometheus metrics with new labels. Key changes include augmenting functions to accept new parameters, updating test cases, adding new validation methods, and modifying configurations to include denomination information. 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64 +/- ##
==========================================
+ Coverage 89.89% 90.16% +0.26%
==========================================
Files 59 58 -1
Lines 2237 2287 +50
==========================================
+ Hits 2011 2062 +51
+ Misses 225 224 -1
Partials 1 1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- pkg/app.go (1 hunks)
- pkg/config/denom_info.go (2 hunks)
- pkg/generators/balance.go (3 hunks)
- pkg/generators/balance_test.go (3 hunks)
Additional context used
GitHub Check: codecov/patch
pkg/app.go
[warning] 113-113: pkg/app.go#L113
Added line #L113 was not covered by tests
Additional comments not posted (3)
pkg/generators/balance.go (2)
16-17: Constructor forBalanceGeneratorproperly initializes theChainsfield.The constructor
NewBalanceGeneratorcorrectly initializes theChainsfield of theBalanceGeneratorstruct. This is a straightforward and effective implementation.
36-77: RefactoredGeneratemethod inBalanceGenerator.The refactoring in the
Generatemethod to iterate overg.Chainsandchain.ConsumerChainsinstead ofdata.Balancesaligns with the structural changes in the data model. This should provide a more robust way of handling balances across multiple chains and their consumers.Ensure proper testing, especially around the nested loops and conditional checks, to prevent any data mishandling.
pkg/generators/balance_test.go (1)
Line range hint
21-94: Updated tests forBalanceGenerator.The updates in the test cases reflect the new structure of
BalanceGeneratorand the data model changes. The tests seem to be comprehensive, covering various scenarios and using assertions to validate the behavior.Ensure that all edge cases, especially error handling and boundary conditions, are covered in the tests.
freak12techno
force-pushed
the
refactor-denoms
branch
from
be14b4b to
c785b11
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- pkg/app.go (1 hunks)
- pkg/generators/commission.go (3 hunks)
- pkg/generators/commission_test.go (3 hunks)
- pkg/generators/rewards.go (3 hunks)
- pkg/generators/rewards_test.go (3 hunks)
- pkg/generators/self_delegation.go (3 hunks)
- pkg/generators/self_delegations_test.go (3 hunks)
Files not reviewed due to errors (3)
- pkg/generators/rewards_test.go (no review received)
- pkg/generators/commission_test.go (no review received)
- pkg/generators/self_delegations_test.go (no review received)
Additional context used
Learnings (1)
pkg/app.go (1)
User: freak12techno PR: QuokkaStake/cosmos-validators-exporter#64 File: pkg/app.go:113-113 Timestamp: 2024-06-23T21:45:53.914Z Learning: The integration of `NewBalanceGenerator` with `appConfig.Chains` in the `App` constructor is implicitly tested in the `TestBalanceGeneratorNotEmptyState` test case. This test case checks the behavior of `NewBalanceGenerator` when provided with specific chain configurations.
GitHub Check: codecov/patch
pkg/app.go
[warning] 108-108: pkg/app.go#L108
Added line #L108 was not covered by tests
[warning] 112-114: pkg/app.go#L112-L114
Added lines #L112 - L114 were not covered by tests
Additional comments not posted (6)
pkg/generators/rewards.go (2)
16-17: Constructor correctly initializesChainsfield.The new constructor implementation properly initializes the
RewardsGeneratorwith the given chain configurations, aligning with the PR's refactoring goals.
36-54: Enhanced loop logic to handle multiple chains and currency conversion.The updated loop in the
Generatemethod effectively iterates over multiple chain configurations, utilizing the newConvertmethod for accurate denomination conversions. This is a significant improvement for scalability and flexibility in handling various chain specifics.pkg/generators/self_delegation.go (2)
16-17: Constructor correctly initializesChainsfield inSelfDelegationGenerator.The constructor for
SelfDelegationGeneratoris correctly updated to initialize theChainsfield, ensuring that chain configurations are properly passed to the generator.
36-53: UpdatedGeneratemethod to support multiple chain configurations.The
Generatemethod inSelfDelegationGeneratorhas been effectively updated to iterate over multiple chains and perform currency conversions using the newly introducedConvertmethod. This enhances the functionality to accommodate varying chain specifics.pkg/generators/commission.go (2)
16-17: Proper initialization ofChainsfield inCommissionGenerator.The constructor for
CommissionGeneratorhas been updated to initialize theChainsfield, which is crucial for handling multiple chain configurations effectively.
36-54: UpdatedGeneratemethod to handle chain-specific commission data.The
Generatemethod inCommissionGeneratorhas been updated to handle multiple chains, using theConvertmethod for accurate currency conversions. This ensures that the generator can adapt to different chain configurations and their specific commission data.
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (3)
pkg/generators/rewards.go (1)
13-13: Add documentation forChainsfield.The
Chainsfield inRewardsGeneratorstruct is missing documentation. It's best practice to document public fields, especially in a struct that is likely central to your application logic.pkg/generators/self_delegation.go (1)
13-13: Add documentation forChainsfield.Similar to the
RewardsGenerator, theChainsfield inSelfDelegationGeneratorstruct lacks documentation. Adding a comment here would improve code readability and maintainability.pkg/generators/commission.go (1)
13-13: Add documentation forChainsfield.The
Chainsfield inCommissionGeneratorstruct is also missing documentation. Consistent with other generator files, adding a comment here would be beneficial.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- pkg/app.go (1 hunks)
- pkg/config/denom_info.go (2 hunks)
- pkg/fetchers/price.go (1 hunks)
- pkg/fetchers/price_test.go (5 hunks)
- pkg/generators/balance.go (3 hunks)
- pkg/generators/balance_test.go (3 hunks)
- pkg/generators/commission.go (3 hunks)
- pkg/generators/commission_test.go (3 hunks)
- pkg/generators/rewards.go (3 hunks)
- pkg/generators/rewards_test.go (3 hunks)
- pkg/generators/self_delegation.go (3 hunks)
- pkg/generators/self_delegations_test.go (3 hunks)
- pkg/generators/validators_info.go (4 hunks)
- pkg/generators/validators_info_test.go (7 hunks)
Files skipped from review as they are similar to previous changes (3)
- pkg/config/denom_info.go
- pkg/generators/balance.go
- pkg/generators/balance_test.go
Additional context used
Learnings (2)
pkg/generators/validators_info_test.go (1)
User: freak12techno PR: QuokkaStake/cosmos-validators-exporter#63 File: pkg/fetchers/consumer_validators.go:40-40 Timestamp: 2024-06-23T16:11:23.143Z Learning: The `TestConsumerValidatorsFetcherBase` method in `pkg/fetchers/consumer_validators_test.go` tests the initialization of `ConsumerValidatorsFetcher` with the `Chains` parameter.pkg/app.go (1)
User: freak12techno PR: QuokkaStake/cosmos-validators-exporter#64 File: pkg/app.go:113-113 Timestamp: 2024-06-23T21:45:53.914Z Learning: The integration of `NewBalanceGenerator` with `appConfig.Chains` in the `App` constructor is implicitly tested in the `TestBalanceGeneratorNotEmptyState` test case. This test case checks the behavior of `NewBalanceGenerator` when provided with specific chain configurations.
GitHub Check: codecov/patch
pkg/app.go
[warning] 106-106: pkg/app.go#L106
Added line #L106 was not covered by tests
[warning] 110-113: pkg/app.go#L110-L113
Added lines #L110 - L113 were not covered by tests
Additional comments not posted (17)
pkg/generators/rewards.go (1)
16-17: Constructor function implementation looks good.The constructor function
NewRewardsGeneratorcorrectly initializes theRewardsGeneratorwith the provided chains. This is a straightforward and clean implementation.pkg/generators/self_delegation.go (1)
16-17: Constructor function implementation is correct.The constructor for
SelfDelegationGeneratoris implemented correctly, initializing the struct with the provided chains. This setup is consistent with other generator classes in the module.pkg/generators/commission.go (1)
16-17: Constructor function is implemented correctly.The
NewCommissionGeneratorconstructor is correctly set up to initialize theCommissionGeneratorwith the provided chains. This consistency across generator constructors is good to see.pkg/generators/rewards_test.go (2)
41-53: Review ofchainsslice setup in test cases.The setup of the
chainsslice in the test cases is done correctly. It reflects the new structure expected by theRewardsGenerator. This setup is crucial for testing the generator with different chain configurations.
60-68: Review of assertions in test cases.The assertions in the test cases are well-structured. They correctly use the
InEpsilonmethod to account for floating point precision issues, which is important when dealing with financial calculations.pkg/generators/commission_test.go (2)
21-21: LGTM!The changes correctly reflect the updated
NewCommissionGeneratorfunction signature to accommodate chain configurations.
54-54: LGTM!The generator is correctly initialized with a non-empty list of chains, and the results are appropriately asserted.
pkg/generators/self_delegations_test.go (2)
21-21: LGTM!The changes correctly reflect the updated
NewSelfDelegationGeneratorfunction signature to accommodate chain configurations.
56-56: LGTM!The generator is correctly initialized with a non-empty list of chains, and the results are appropriately asserted.
pkg/fetchers/price.go (1)
78-78: LGTM!The update to use
DisplayDenominstead ofDenomfor currency rates mapping is correctly implemented and aligns with the changes to the data model.pkg/generators/validators_info.go (2)
20-20: LGTM!The
NewValidatorsInfoGeneratorfunction is correctly updated to take a list of chains as a parameter, aligning with the new architecture requirements.
Line range hint
54-83: LGTM!The
Generatemethod correctly handles chain-specific data and uses the newConvertmethod for denomination conversions, ensuring data consistency across different chain configurations.pkg/generators/validators_info_test.go (3)
22-22: Refactor: UpdateNewValidatorsInfoGeneratorinitialization to use chain configurationsThe updated test cases correctly initialize
NewValidatorsInfoGeneratorwith the new chain configurations. This change is consistent across multiple test functions, ensuring that the generator's behavior is tested against the intended configurations.Also applies to: 32-32, 65-65, 104-104
67-69: Refactor: Chain configuration and assertions updatedThe addition of detailed chain configurations and the update of assertions to include the
"denom": "atom"label are well-integrated. These changes ensure that the tests are aligned with the new chain-specific logic in the generator.
[APROVED]Also applies to: 84-84
46-46: Adjustment: UpdatedDelegatorSharesvalues in test validatorsThe
DelegatorSharesvalues for validators have been updated to new values. It's important to verify that these values are consistent with expected test scenarios and reflect realistic test conditions.Also applies to: 51-51, 55-55
pkg/fetchers/price_test.go (2)
54-54: Refactor: UpdatedDenomsconfiguration in price fetcher testsThe tests have been updated to reflect the new
Denomsconfiguration, which includes aDisplayDenomfield. This change is crucial for ensuring that the price fetcher correctly handles the new denomination display names.Also applies to: 92-92, 135-135
115-115: Validation: Correct handling of denomination data in price fetcherThe assertions for
denomDatacorrectly check the fetched price data against the expected values. This ensures that the price fetcher is accurately processing and returning the correct price information for the specified denominations.Also applies to: 159-159
| for _, chain := range g.Chains { | ||
| chainRewards, ok := data.Rewards[chain.Name] | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| for _, validator := range chain.Validators { | ||
| validatorRewards, ok := chainRewards[validator.Address] | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| for _, balance := range validatorRewards { | ||
| amountConverted := chain.Denoms.Convert(&balance) | ||
| selfDelegationRewardsTokens.With(prometheus.Labels{ | ||
| "chain": chain, | ||
| "address": validator, | ||
| "denom": balance.Denom, | ||
| }).Set(balance.Amount) | ||
| "chain": chain.Name, | ||
| "address": validator.Address, | ||
| "denom": amountConverted.Denom, | ||
| }).Set(amountConverted.Amount) |
There was a problem hiding this comment.
Review of loop logic in Generate method.
The nested loops handle the chain-specific rewards data correctly. However, consider adding error handling or logging when the ok check fails, as silently continuing might obscure data issues.
+ if !ok {
+ log.Printf("No rewards data available for chain %s", chain.Name)
+ continue
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, chain := range g.Chains { | |
| chainRewards, ok := data.Rewards[chain.Name] | |
| if !ok { | |
| continue | |
| } | |
| for _, validator := range chain.Validators { | |
| validatorRewards, ok := chainRewards[validator.Address] | |
| if !ok { | |
| continue | |
| } | |
| for _, balance := range validatorRewards { | |
| amountConverted := chain.Denoms.Convert(&balance) | |
| selfDelegationRewardsTokens.With(prometheus.Labels{ | |
| "chain": chain, | |
| "address": validator, | |
| "denom": balance.Denom, | |
| }).Set(balance.Amount) | |
| "chain": chain.Name, | |
| "address": validator.Address, | |
| "denom": amountConverted.Denom, | |
| }).Set(amountConverted.Amount) | |
| for _, chain := range g.Chains { | |
| chainRewards, ok := data.Rewards[chain.Name] | |
| if !ok { | |
| log.Printf("No rewards data available for chain %s", chain.Name) | |
| continue | |
| } | |
| for _, validator := range chain.Validators { | |
| validatorRewards, ok := chainRewards[validator.Address] | |
| if !ok { | |
| continue | |
| } | |
| for _, balance := range validatorRewards { | |
| amountConverted := chain.Denoms.Convert(&balance) | |
| selfDelegationRewardsTokens.With(prometheus.Labels{ | |
| "chain": chain.Name, | |
| "address": validator.Address, | |
| "denom": amountConverted.Denom, | |
| }).Set(amountConverted.Amount) |
There was a problem hiding this comment.
We can have this if the query for commission is disabled and this is not an error.
There was a problem hiding this comment.
No description provided.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- pkg/generators/single_validator_info.go (2 hunks)
- pkg/generators/single_validator_info_test.go (4 hunks)
Additional comments not posted (4)
pkg/generators/single_validator_info.go (1)
98-98: Addition of 'denom' label indelegationsGaugemetrics is consistent with PR objectives.This change aligns with the overall objective of enhancing the flexibility of the generators by incorporating chain-specific configurations. The addition of the 'denom' label allows more detailed metric tracking per denomination.
pkg/generators/single_validator_info_test.go (3)
78-79: Proper configuration and initialization ofDenomInfosin tests.The test setup correctly initializes
DenomInfoswith appropriate values, which is crucial for ensuring that the conversion of denominations is tested properly. This reflects the changes made in the main codebase and helps maintain consistency between implementation and testing.
88-93: Comprehensive testing of validator statuses and properties.These tests effectively cover various scenarios, including different validator statuses and commission rates. It's good to see that the tests are thorough, covering multiple aspects of the validator information. This is essential for ensuring that the generator behaves as expected across different conditions.
[APROVED]Also applies to: 112-112
176-176: Correct denomination in test assertions.The test correctly asserts the expected denomination ('atom') after conversion. This ensures that the
Convertmethod is functioning as intended and that the metrics are being recorded with the correct denominations.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- config.example.toml (3 hunks)
- pkg/app.go (1 hunks)
- pkg/config/denom_info.go (3 hunks)
- pkg/generators/active_set_tokens.go (2 hunks)
- pkg/generators/active_set_tokens_test.go (3 hunks)
- pkg/generators/balance_test.go (3 hunks)
- pkg/generators/commission_test.go (3 hunks)
- pkg/generators/rewards_test.go (3 hunks)
- pkg/generators/self_delegations_test.go (3 hunks)
- pkg/generators/single_validator_info_test.go (4 hunks)
- pkg/generators/validators_info_test.go (7 hunks)
Files skipped from review as they are similar to previous changes (8)
- pkg/app.go
- pkg/config/denom_info.go
- pkg/generators/balance_test.go
- pkg/generators/commission_test.go
- pkg/generators/rewards_test.go
- pkg/generators/self_delegations_test.go
- pkg/generators/single_validator_info_test.go
- pkg/generators/validators_info_test.go
Additional context used
Learnings (2)
pkg/generators/active_set_tokens.go (1)
User: freak12techno PR: QuokkaStake/cosmos-validators-exporter#62 File: pkg/generators/active_set_tokens.go:61-61 Timestamp: 2024-06-22T22:57:51.216Z Learning: The indirect testing method used in `ActiveSetTokensGenerator`, which verifies the outcome by checking the metric's value against the expected stake of the last active validator, is considered sufficient by the user for reflecting the intended behavior of the sorting logic.pkg/generators/active_set_tokens_test.go (1)
User: freak12techno PR: QuokkaStake/cosmos-validators-exporter#62 File: pkg/generators/active_set_tokens.go:61-61 Timestamp: 2024-06-22T22:57:51.216Z Learning: The indirect testing method used in `ActiveSetTokensGenerator`, which verifies the outcome by checking the metric's value against the expected stake of the last active validator, is considered sufficient by the user for reflecting the intended behavior of the sorting logic.
Additional comments not posted (5)
pkg/generators/active_set_tokens.go (2)
42-42: Extended metric labels to include 'denom'The addition of the "denom" label to the Prometheus metric definition is consistent with the PR's objectives to enhance denomination tracking. This change allows for more granular monitoring and is a good use of Prometheus's capabilities.
65-74: Logic for denomination conversion and metric settingThe changes to convert the last validator's stake using the
DenomInfos.Convertmethod and set the metric values accordingly are well-implemented. This ensures that the metrics reflect the denominations accurately. Make sure that theConvertmethod handles edge cases and errors robustly.pkg/generators/active_set_tokens_test.go (2)
Line range hint
76-108: Updated test setup with detailed chain and denomination informationThe modifications in the test setup to include detailed chain and denomination information are crucial for ensuring the generator's functionality with the new denomination features. The use of realistic data in
DelegatorSharesenhances the test's reliability. Ensure that all edge cases for denomination conversion are covered in the tests.
Line range hint
115-147: Enhanced test setup for sufficient validators scenarioThe test setup for scenarios with enough validators now includes updated
DelegatorSharesvalues and detailed denomination data, aligning with the changes in the main code. This ensures that the tests are comprehensive and reflect the operational conditions accurately.config.example.toml (1)
Line range hint
2-125: Comprehensive update to configuration settingsThe updates to the configuration file are comprehensive and align well with the new features introduced in the codebase. The detailed settings for denominations, including base and display denominations, are particularly noteworthy. These changes will facilitate more accurate and customizable metric generation based on the chain-specific settings.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- pkg/config/chain.go (3 hunks)
- pkg/config/chain_test.go (5 hunks)
- pkg/config/config_test.go (3 hunks)
- pkg/config/consumer_chain.go (2 hunks)
- pkg/config/consumer_chain_test.go (2 hunks)
- pkg/fetchers/signing_info.go (1 hunks)
- pkg/fetchers/signing_info_test.go (5 hunks)
Additional comments not posted (11)
pkg/config/consumer_chain_test.go (1)
48-54: Ensure error handling for missingBaseDenominConsumerChain.The addition of this test case is crucial as it verifies the new validation logic ensuring that
BaseDenomis set, aligning with the enhanced configuration requirements.pkg/config/consumer_chain.go (2)
45-47: Added validation forBaseDenom.This change is essential for ensuring that all consumer chains have a base denomination set, which is crucial for operations that depend on currency values. This aligns with the new configuration requirements.
58-83: Enhanced warning generation for missing prefixes.The added warnings for missing
BechWalletPrefix,BechValidatorPrefix, andBechConsensusPrefixare important for alerting users about potential configuration issues that could affect the application's functionality. This proactive error handling is a good practice.pkg/config/chain.go (2)
48-50: Validate presence ofBaseDenom.This validation is crucial for ensuring that the base denomination is set for each chain, which is necessary for financial calculations and operations within the application.
Line range hint
76-94: Comprehensive warning management for chains.The modifications to include warnings for missing
BechWalletPrefixand the addition of iterating overConsumerChainsto gather their warnings ensure that all potential configuration issues are flagged. This is a robust approach to handling configuration validation.pkg/config/config_test.go (2)
49-49: Validate configuration withBaseDenom.The test ensures that the configuration is valid when
BaseDenomis set, which is crucial for the application's operations that depend on currency values.
65-65: Check for warnings related toBaseDenomandBechWalletPrefix.The tests for warnings when
BaseDenomandBechWalletPrefixare set are important to ensure that the application alerts users about potential configuration issues. It's good to see that these aspects are being thoroughly tested.Also applies to: 79-83
pkg/config/chain_test.go (1)
48-54: Comprehensive testing ofChainvalidation and warning generation.The tests cover various scenarios including missing
BaseDenom, invalid validators, and consumer chains. This ensures that theChainstruct's validation logic is robust and can handle different configuration errors effectively.Also applies to: 62-62, 75-75, 89-89, 104-104, 118-119, 143-151, 158-163
pkg/fetchers/signing_info.go (1)
148-148: Ensure proper handling when Bech prefixes are missing.The condition
if chain.BechConsensusPrefix == "" || chain.BechValidatorPrefix == ""correctly prevents further operations if required Bech prefixes are not set, which is a good practice for robust error handling.pkg/fetchers/signing_info_test.go (2)
344-344: Consistent use of BechValidatorPrefix in test setups.The addition of
BechValidatorPrefixin the test setups for consumer chains is consistent across multiple test cases. This consistency is crucial for ensuring that the tests accurately reflect the expected real-world usage of the application.Also applies to: 401-401, 449-449
369-369: Verify the handling of empty data in error scenarios.The tests
TestSigningInfoFetcherConsumerAssignedKeyQueryErrorandTestSigningInfoFetcherConsumerAssignedKeyNodeErrorare designed to handle error scenarios properly by ensuring that no data is populated when an error occurs. This is a good practice to prevent corrupt or partial data from being used.Also applies to: 426-426
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores