fix: Fix inactive vals and min stake handling in ConsumerAdditionProposal conversion #2181
Conversation
github-actions
bot
added
C:Testing
WalkthroughWalkthroughThe changes introduce two new fields, Changes
Possibly related issues
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 Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (2)
x/ccv/provider/keeper/proposal.go (1)
47-48: Review: Addition ofMinStakeandAllowInactiveValsfields toConsumerAdditionProposalThe addition of
MinStakeandAllowInactiveValsfields to theConsumerAdditionProposalstruct is a significant change. These fields allow for more granular control over validator participation in consumer chains, which is crucial for maintaining the integrity and security of the interchain operations.
MinStake: This field likely sets a threshold for the minimum stake that validators must have to participate, which is important for ensuring that only sufficiently backed validators can influence the chain.AllowInactiveVals: This field allows proposals to include inactive validators. This could be useful in scenarios where validators are temporarily inactive but still trusted based on past interactions.Considerations:
- Ensure that the rest of the system correctly interprets these new fields, particularly in functions that process these proposals.
- It might be beneficial to add validation logic within this function or upstream to ensure that the values provided for these fields meet system-wide business rules (e.g.,
MinStakeshould be non-negative).Code Quality:
- The code adheres to the Uber Golang style guide, with proper naming conventions and struct initialization.
- Error handling and propagation are consistent with the rest of the module's practices.
The changes are well-integrated and follow the project's coding standards.
Consider implementing additional validation for the new fields if not already present elsewhere in the system to prevent invalid data from being processed without detection.
tests/e2e/actions.go (1)
637-638: Best Practice: Avoid Magic NumbersThe JSON template in
submitConsumerModificationProposaluses magic numbers directly in the string.Replace these magic numbers with named constants or configuration values to make the code more readable and maintainable.
- "min_stake": %d, - "allow_inactive_vals": %t + "min_stake": %d, // use a named constant or configuration value + "allow_inactive_vals": %t // use a named constant or configuration valueDefine these constants at the beginning of your file or fetch them from a configuration management system.
| "min_stake": %d, | ||
| "allow_inactive_vals": %t |
There was a problem hiding this comment.
Code Quality Improvement: Use Struct for Parameters
The function submitConsumerModificationProposal uses many parameters directly in the JSON template string. This can be error-prone and hard to maintain.
Consider defining a struct for these parameters and marshal it to JSON. This approach enhances code readability and maintainability.
type ConsumerModificationParams struct {
MinStake uint64 `json:"min_stake"`
AllowInactiveVals bool `json:"allow_inactive_vals"`
// include other fields...
}
params := ConsumerModificationParams{
MinStake: action.MinStake,
AllowInactiveVals: action.AllowInactiveVals,
// set other fields...
}
jsonStr, err := json.Marshal(params)
if err != nil {
log.Fatal(err)
}Use this jsonStr in your template.
| AllowInactiveVals bool | ||
| MinStake uint64 |
There was a problem hiding this comment.
Refactor Suggestion: Consistent Parameter Ordering
The addition of AllowInactiveVals and MinStake parameters in SubmitConsumerModificationProposalAction struct should follow a consistent order across similar functions to improve readability and maintainability.
Consider reordering the parameters to match the order used in other related structs or function signatures for consistency.
type SubmitConsumerModificationProposalAction struct {
Chain ChainID
From ValidatorID
Deposit uint
ConsumerChain ChainID
TopN uint32
ValidatorsPowerCap uint32
ValidatorSetCap uint32
Allowlist []string
Denylist []string
+ AllowInactiveVals bool
+ MinStake uint64
- AllowInactiveVals bool
- MinStake uint64
}Committable suggestion was skipped due to low confidence.
| @@ -299,7 +299,9 @@ | |||
| "validator_set_cap": %d, | |||
| "allowlist": %s, | |||
| "denylist": %s, | |||
| "authority": "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn" | |||
| "authority": "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn", | |||
There was a problem hiding this comment.
Security Issue: Exposed API Key
The hardcoded API key in the JSON template might expose sensitive operations to unauthorized access.
Consider removing the hardcoded API key or replacing it with a secure method of fetching it, such as environment variables or secure vault solutions.
- "authority": "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn",
+ "authority": os.Getenv("AUTHORITY_KEY"),Ensure to import the os package at the beginning of the file.
Committable suggestion was skipped due to low confidence.
Tools
Gitleaks
302-302: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Fixes how consumer proposals are constructed.
The main changes are:
Summary by CodeRabbit