Enable permissioned keys by default #2645
Conversation
WalkthroughThe changes in this pull request primarily involve updates to configuration files related to the Changes
Possibly related PRs
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (3)
protocol/x/accountplus/types/genesis.go (1)
Line range hint
13-15: Consider adding validation for IsSmartAccountActive.The
Validatefunction currently performs no validation. Consider adding validation to ensureIsSmartAccountActiveis properly initialized.func (gs GenesisState) Validate() error { + // Validate IsSmartAccountActive is properly initialized + if gs.Params.IsSmartAccountActive { + // Add any specific validation rules for when smart accounts are active + } return nil }protocol/app/upgrades/v8.0/migrate_accountplus_test.go (2)
28-37: Add documentation explaining the test setup rationaleThe test setup explicitly sets
IsSmartAccountActiveto false, presumably to test the migration from disabled to enabled state. This setup intention should be documented for clarity.Consider adding a comment like:
func (s *UpgradeTestSuite) SetupTest() { + // Initialize with IsSmartAccountActive=false to test migration to enabled state s.tApp = testapp.NewTestAppBuilder(s.T()).WithGenesisDocFn(func() (genesis types.GenesisDoc) {
Line range hint
41-93: Consider adding assertions for smart account activation stateThe test thoroughly verifies the storage migration but doesn't assert the final state of
IsSmartAccountActive. Consider adding verification of the smart account activation state after migration.Add assertions like:
} + // Verify smart account activation state after migration + params := s.tApp.App.AccountPlusKeeper.GetParams(ctx) + s.Require().True(params.IsSmartAccountActive, "Smart accounts should be active after migration") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
protocol/app/upgrades/v8.0/migrate_accountplus_test.go(2 hunks)protocol/testing/genesis.sh(1 hunks)protocol/x/accountplus/types/genesis.go(1 hunks)
🔇 Additional comments (4)
protocol/x/accountplus/types/genesis.go (1)
5-9: LGTM! Verify the impact of enabling smart accounts by default.
The change correctly initializes GenesisState with IsSmartAccountActive set to true, which enables permissioned keys by default as intended.
✅ Verification successful
Based on the search results, I can now generate the final response:
Smart account activation by default is safe and well-integrated
The change to enable smart accounts by default (IsSmartAccountActive = true) is well-supported by the codebase:
- The parameter is properly handled in the keeper layer with getter/setter methods
- Circuit breaker and authentication flows properly respect this flag
- Tests cover both active and inactive states
- The feature can be toggled via governance if needed (
MsgSetActiveState)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the impact of enabling smart accounts by default.
# Test: Search for any references to IsSmartAccountActive to understand usage.
rg -A 5 "IsSmartAccountActive"
# Test: Search for any upgrade handlers that might need to handle this change.
ast-grep --pattern $'func CreateUpgradeHandler($_, $_) {
$$$
}'
Length of output: 16645
protocol/testing/genesis.sh (1)
2155-2157: LGTM! Consistent with genesis.go changes.
The script correctly configures the accountplus module by setting is_smart_account_active to true in the genesis state, which aligns with the changes in genesis.go.
protocol/app/upgrades/v8.0/migrate_accountplus_test.go (2)
6-6: LGTM: Import addition is appropriate
The new import for CometBFT types is necessary for the genesis document manipulation in the test setup.
28-37: Verify test coverage for smart account activation
While this test covers the account state migration, we should verify that the activation of smart accounts is also tested.
✅ Verification successful
Let me check the content of these files to understand the test coverage better.
Let me check specifically for tests related to the v8.0 upgrade migration to ensure we have proper coverage.
Let me check the content of the upgrade file and all test files in the v8.0 directory to understand the migration and its test coverage completely.
Smart account activation is properly tested across the codebase
Looking at the code and test coverage:
- The v8.0 upgrade explicitly sets smart accounts to active with
accountplusKeeper.SetActiveState(sdkCtx, true)in the upgrade handler - The migration test properly sets up the initial state with
IsSmartAccountActive = false - Comprehensive test coverage for smart account activation exists in:
protocol/x/accountplus/ante/circuit_breaker_test.go: Tests activation/deactivation behaviorprotocol/x/accountplus/keeper/keeper_test.go: Tests keeper-level activation stateprotocol/x/accountplus/genesis_test.go: Tests genesis state with active smart accounts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests covering the smart account activation
rg -l "IsSmartAccountActive.*true" --type go
Length of output: 222
Script:
#!/bin/bash
# Check the content of test files for smart account activation testing
rg "IsSmartAccountActive.*true" -B 2 -A 2 --type go
Length of output: 1675
Script:
#!/bin/bash
# Search for all test files related to v8.0 upgrade and smart account migration
rg -l "migrate.*account" protocol/app/upgrades/v8.0/
Length of output: 92
Script:
#!/bin/bash
# List all files in v8.0 directory and show their content
echo "=== Files in v8.0 directory ==="
ls protocol/app/upgrades/v8.0/
echo -e "\n=== Content of upgrade.go ==="
cat protocol/app/upgrades/v8.0/upgrade.go
echo -e "\n=== Content of migrate_accountplus_test.go ==="
cat protocol/app/upgrades/v8.0/migrate_accountplus_test.go
Length of output: 7074
|
@Mergifyio backport release/protocol/v8.x |
✅ Backports have been created
|
(cherry picked from commit 3d59131)
Co-authored-by: jayy04 <[email protected]>
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests