fix(x/accounts/lockup): fix proto path #22319
Conversation
julienrbrt
requested review from
testinginprod,
sontrinh16 and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a significant restructuring of the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (3)
🔇 Additional comments (5)
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
Documentation and Community
|
|
@julienrbrt your pull request is missing a changelog! |
![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 (14)
x/accounts/defaults/lockup/permanent_locking_account_test.go (2)
Line range hint
33-49: Consider adding more test cases for edge scenarios.The current test case covers a successful delegation scenario. To improve test coverage, consider adding test cases for:
- Delegation with invalid sender
- Delegation with invalid validator address
- Delegation with zero or negative amount
- Delegation exceeding available balance
These additional tests will help ensure the robustness of the delegation functionality.
Line range hint
51-82: Enhance test coverage for undelegation scenarios.The current test case covers a successful undelegation scenario. To improve test coverage, consider adding test cases for:
- Undelegation with invalid sender
- Undelegation with invalid validator address
- Undelegation with zero or negative amount
- Undelegation exceeding delegated amount
- Multiple undelegations
These additional tests will help ensure the robustness of the undelegation functionality.
x/accounts/defaults/lockup/continuous_locking_account_test.go (1)
Line range hint
1-265: LGTM: Comprehensive test coverage with a minor suggestion.The test suite provides excellent coverage for the ContinuousLockingAccount functionality. It includes tests for delegation, undelegation, sending coins, withdrawing unlocked coins, and retrieving lock coin info. The tests cover various scenarios, including time-dependent behavior and error cases.
Consider standardizing the spelling of "Continuous" in the test function names. Currently, it's spelled as "Continous" in all test function names. Updating this would improve consistency:
-func TestContinousAccountDelegate(t *testing.T) { +func TestContinuousAccountDelegate(t *testing.T) { -func TestContinousAccountUndelegate(t *testing.T) { +func TestContinuousAccountUndelegate(t *testing.T) { -func TestContinousAccountSendCoins(t *testing.T) { +func TestContinuousAccountSendCoins(t *testing.T) { -func TestContinousAccountWithdrawUnlockedCoins(t *testing.T) { +func TestContinuousAccountWithdrawUnlockedCoins(t *testing.T) { -func TestContinousAccountGetLockCoinInfo(t *testing.T) { +func TestContinuousAccountGetLockCoinInfo(t *testing.T) {x/accounts/defaults/lockup/lockup_test.go (1)
Line range hint
1-324: LGTM: Comprehensive test coverage with well-structured test cases.The test suite for the lockup module is well-designed:
- Uses table-driven tests for efficient scenario testing.
- Covers both positive and negative cases, including edge conditions.
- Properly tests error scenarios.
- Follows Go testing best practices.
Consider adding more descriptive names for test cases. For example, in TestTrackingDelegation, instead of:
"delegate amount less than the vesting amount",You could use:
"should succeed: delegate amount less than vesting amount",This naming convention provides more context about the expected outcome of each test case at a glance.
x/accounts/defaults/lockup/periodic_locking_account_test.go (2)
Line range hint
1-329: Consider adding a simple test for the new import.While the existing tests provide good coverage for the PeriodicLockingAccount functionality, it might be beneficial to add a simple test to ensure the new import is working correctly. This could help catch any potential issues related to the package restructuring.
Consider adding a test like this:
func TestLockupTypesImport(t *testing.T) { // This test doesn't need to do anything, it just ensures that the new import compiles correctly _ = lockuptypes.MsgInitPeriodicLockingAccount{} }
Line range hint
1-329: Minor style improvements suggested.The code generally conforms well to the Uber Golang style guide. However, there are a few minor improvements that could be made:
- Consider using
t.Cleanup()for teardown operations instead of relying ondeferstatements in test helper functions.- Use
assertpackage alongsiderequirefor more expressive test assertions where appropriate.- Consider grouping related tests using
t.Run()for better organization and potential parallelization.These changes are not critical but could improve the overall code quality and test structure.
x/accounts/README.md (7)
37-38: Minor grammatical correction neededConsider rephrasing the bullet points for clarity:
- One located at address "cosmos123"
- Another located at address "cosmos456"
This small change improves the readability of the example.
54-55: Enhance clarity of Init method explanationConsider expanding on the comparison to other smart contract systems:
- The
Initmethod is analogous to theinstantiatemethod in a CosmWasm contract- It's similar to the
constructorin an EVM contract- Both are used to set up the initial state when an account (or contract) is created
This additional context helps readers understand the purpose and importance of the Init method.
109-110: Clarify execute handler triggersConsider expanding on when execute handlers can be triggered:
- During block execution (not queries) through transactions
- During begin or end block (if applicable)
- Potentially by other system processes (if relevant)
This additional information provides a more comprehensive understanding of execute handlers' role in the system.
179-180: Enhance explanation of query handler invocationConsider expanding on who can access query handlers:
- External clients (e.g., CLI, wallets, dApps)
- Other modules and accounts within the system
- Potentially during certain blockchain operations (if applicable)
This additional context helps readers understand the versatility and importance of query handlers.
434-434: Enhance security considerations for authenticationConsider adding more security best practices for authentication implementation:
- Prevent replay attacks by making it impossible to reuse the same action with the same signature
- Implement rate limiting to prevent brute-force attempts
- Use secure random number generation for any nonce or challenge-response mechanisms
- Ensure proper error handling to avoid leaking sensitive information
These additional security considerations will help developers implement more robust authentication mechanisms.
494-495: Clarify when to implement AuthRetroCompatibilityConsider expanding on when to implement the AuthRetroCompatibility method:
- Implement this handler only for account types you want to expose via x/auth gRPC methods
- This is particularly useful for ensuring compatibility with existing wallets that have not yet integrated with x/accounts
- The
infofield in the response can be nil if your account doesn't fit theBaseAccountstructureThis additional context helps developers understand when and why they might need to implement this method.
Line range hint
506-506: Clarify CLI command for generating init messagesConsider expanding on the CLI command usage:
- Explain what each parameter represents (e.g., [account type], [msg])
- Provide a concrete example of how to use the command
- Mention any limitations or considerations when using this command
This additional information will help users better understand how to use the CLI command for generating init messages.
api/cosmos/accounts/defaults/lockup/v1/lockup.pulsar.go (1)
Line range hint
2-763: Avoid modifying generated code directlyThis file is generated by
protoc-gen-go-pulsaras indicated by the comment at the top:// Code generated by protoc-gen-go-pulsar. DO NOT EDIT.Modifying generated code is discouraged because changes will be overwritten the next time the code is regenerated. Instead, make necessary changes in the source.protofiles and regenerate the code.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (6)
x/accounts/defaults/lockup/types/lockup.pb.gois excluded by!**/*.pb.gox/accounts/defaults/lockup/types/query.pb.gois excluded by!**/*.pb.gox/accounts/defaults/lockup/types/tx.pb.gois excluded by!**/*.pb.gox/accounts/defaults/lockup/v1/lockup.pb.gois excluded by!**/*.pb.gox/accounts/defaults/lockup/v1/query.pb.gois excluded by!**/*.pb.gox/accounts/defaults/lockup/v1/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (18)
- api/cosmos/accounts/defaults/lockup/v1/lockup.pulsar.go (15 hunks)
- api/cosmos/accounts/defaults/lockup/v1/query.pulsar.go (51 hunks)
- tests/e2e/accounts/lockup/periodic_lockup_test_suite.go (1 hunks)
- tests/e2e/accounts/lockup/utils.go (1 hunks)
- x/accounts/README.md (8 hunks)
- x/accounts/defaults/lockup/continuous_locking_account.go (1 hunks)
- x/accounts/defaults/lockup/continuous_locking_account_test.go (1 hunks)
- x/accounts/defaults/lockup/delayed_locking_account.go (1 hunks)
- x/accounts/defaults/lockup/delayed_locking_account_test.go (1 hunks)
- x/accounts/defaults/lockup/lockup.go (1 hunks)
- x/accounts/defaults/lockup/lockup_test.go (1 hunks)
- x/accounts/defaults/lockup/periodic_locking_account.go (1 hunks)
- x/accounts/defaults/lockup/periodic_locking_account_test.go (1 hunks)
- x/accounts/defaults/lockup/permanent_locking_account.go (1 hunks)
- x/accounts/defaults/lockup/permanent_locking_account_test.go (1 hunks)
- x/accounts/proto/cosmos/accounts/defaults/lockup/v1/lockup.proto (1 hunks)
- x/accounts/proto/cosmos/accounts/defaults/lockup/v1/query.proto (1 hunks)
- x/accounts/proto/cosmos/accounts/defaults/lockup/v1/tx.proto (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/accounts/defaults/lockup/lockup.go
🧰 Additional context used
📓 Path-based instructions (14)
api/cosmos/accounts/defaults/lockup/v1/lockup.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/accounts/defaults/lockup/v1/query.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/e2e/accounts/lockup/periodic_lockup_test_suite.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/e2e/accounts/lockup/utils.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/accounts/defaults/lockup/continuous_locking_account.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/defaults/lockup/continuous_locking_account_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/defaults/lockup/delayed_locking_account.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/defaults/lockup/delayed_locking_account_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/defaults/lockup/lockup_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/defaults/lockup/periodic_locking_account.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/defaults/lockup/periodic_locking_account_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/defaults/lockup/permanent_locking_account.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/defaults/lockup/permanent_locking_account_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 buf
x/accounts/proto/cosmos/accounts/defaults/lockup/v1/lockup.proto
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
x/accounts/proto/cosmos/accounts/defaults/lockup/v1/query.proto
4-4: import "cosmos/accounts/defaults/lockup/v1/lockup.proto": file does not exist
(COMPILE)
x/accounts/proto/cosmos/accounts/defaults/lockup/v1/tx.proto
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
🔇 Additional comments (25)
x/accounts/proto/cosmos/accounts/defaults/lockup/v1/lockup.proto (2)
2-2: Approve package versioning changeThe update to include 'v1' in the package name is a good practice. It allows for future versions of the package without breaking existing implementations, enhancing the module's maintainability and extensibility.
4-4: Verify "amino/amino.proto" importThe static analysis tool reports that the "amino/amino.proto" file does not exist. This could be a false positive, but it's worth verifying.
Please run the following command to check if the file exists in the project:
#!/bin/bash # Search for amino.proto file fd amino.protoIf the file is not found, please ensure that the "amino" dependency is correctly set up in your project.
🧰 Tools
🪛 buf
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
x/accounts/proto/cosmos/accounts/defaults/lockup/v1/query.proto (3)
2-2: LGTM: Package declaration updated to include versioning.The package declaration has been correctly updated to include the
v1version, which aligns with the versioning strategy mentioned in the PR objectives.
9-9: LGTM: Go package option updated correctly.The
go_packageoption has been appropriately updated to reflect the new versioning structure. The removal of thetypessuffix aligns with your preference mentioned in the PR description.Regarding your question about naming conventions:
- Using
v1in the package path is a common practice for versioned APIs, especially in gRPC and Protocol Buffers.- While
typesis often used in Go projects,v1is more explicit about versioning and is consistent with the broader ecosystem practices.The choice between
v1andtypesdepends on your project's conventions and future versioning plans. If you anticipate multiple versions in the future, sticking withv1might be more forward-compatible.
4-4: Verify the existence of the imported file.The import path has been updated to reflect the new versioning structure, which is consistent with the package changes. However, the static analysis tool indicates that the file
cosmos/accounts/defaults/lockup/v1/lockup.protodoes not exist.Please ensure that the file
lockup.protohas been moved to the correct location in thev1directory. Run the following command to verify:If the file is not found, please update its location or create it as necessary.
🧰 Tools
🪛 buf
4-4: import "cosmos/accounts/defaults/lockup/v1/lockup.proto": file does not exist
(COMPILE)
x/accounts/defaults/lockup/permanent_locking_account_test.go (2)
Line range hint
1-97: Overall feedback on test coverage and style
- The tests correctly use the updated import path for
lockuptypes.- The test functions follow the Uber Golang style guide with proper error handling and assertions.
- Basic scenarios for delegation, undelegation, and sending coins are covered.
- Consider enhancing test coverage by adding more edge cases and error scenarios for each functionality.
- The style is consistent throughout the file and follows good practices.
Great job on maintaining test consistency while updating the import path. To further improve the quality of this test file, focus on expanding the test cases to cover more scenarios.
14-14: LGTM: Import path updated correctly.The import path for
lockuptypeshas been updated to use the versionedv1package. This change aligns with the PR objective and is a step towards better API versioning.To ensure consistency across the codebase, please run the following command:
✅ Verification successful
Verified: All
lockuptypesimports use thev1path.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all imports of lockuptypes use the new v1 path rg -g '*.go' 'lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/'Length of output: 1193
x/accounts/defaults/lockup/permanent_locking_account.go (2)
Line range hint
1-114: LGTM: Minimal impact on existing code.The import path change has been correctly applied throughout the file without requiring any modifications to the existing code. This suggests a well-structured refactoring of the
lockuptypespackage.To ensure the change doesn't introduce any unexpected issues:
- Run all existing tests related to the
lockupmodule.- Consider adding integration tests that specifically verify the correct functioning of the
PermanentLockingAccountwith the new import.To confirm test coverage, please run:
#!/bin/bash # Description: Check test coverage for the lockup module # Test: Find and run tests related to the lockup module go test -v $(go list ./... | grep 'lockup')This will help ensure that the existing functionality remains intact after the import change.
9-9: LGTM: Import path updated to use versioned package.The change from
lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/types"tolockuptypes "cosmossdk.io/x/accounts/defaults/lockup/v1"aligns with the PR objectives to address import naming inconsistency. This versioned approach is a good practice for managing package versions in Go.To ensure consistency across the codebase, please run the following command:
This will help identify any files that might still be using the old import path and confirm the widespread adoption of the new path.
x/accounts/proto/cosmos/accounts/defaults/lockup/v1/tx.proto (5)
2-2: LGTM: Package versioning added.The addition of
v1to the package name is a good practice for maintaining backwards compatibility and aligns with the PR objectives.
6-6: LGTM: Import path updated to match package versioning.The import path change is consistent with the package name update, ensuring the correct version of the lockup proto file is used.
13-13: LGTM: Go package option updated for consistency.The go_package option change aligns with the package name and import path updates. Regarding your question about naming conventions:
- Using
v1in the package name is a common practice for versioning in Protocol Buffers and Go modules. It allows for future backwards-incompatible changes without breaking existing clients.- Removing the
typessuffix and usingv1instead is consistent with many Go projects and the Google API design guide.This approach balances versioning needs with clean import paths, addressing your preference while maintaining consistency with common practices.
Line range hint
1-148: Summary: Versioning changes improve module structure.The changes in this file consistently implement versioning across the package name, import paths, and go_package option. These modifications:
- Enhance maintainability by clearly separating different versions of the module.
- Improve compatibility management for future updates.
- Align with common practices in Protocol Buffer and Go module versioning.
No changes were made to the message definitions or their fields, preserving the existing functionality while improving the overall structure.
🧰 Tools
🪛 buf
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
4-4: Verify the "amino/amino.proto" import.The static analysis tool reports that "amino/amino.proto" does not exist. While this could be a false positive, please verify that this file exists and is in the correct location in your project structure.
✅ Verification successful
The "amino/amino.proto" import is valid.
The file
proto/amino/amino.protoexists and is correctly referenced in the import statement.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the amino/amino.proto file exists in the project # Search for the file fd -t f 'amino.proto' # If not found, check if it's defined in a buf.yaml file if [ $? -ne 0 ]; then echo "amino.proto not found directly. Checking buf.yaml files:" fd 'buf.yaml' -x grep -H 'amino/amino.proto' fiLength of output: 63
🧰 Tools
🪛 buf
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
x/accounts/defaults/lockup/delayed_locking_account_test.go (1)
Line range hint
1-238: Verify test coverage and execution with the updated import.The existing tests provide good coverage for the DelayedLockingAccount functionality. However, with the import path change, it's crucial to ensure that all tests are still passing.
Please run the following command to execute the tests and verify their success:
This will help confirm that the import path change hasn't introduced any issues and that the test coverage remains intact.
x/accounts/defaults/lockup/continuous_locking_account.go (1)
11-11: LGTM: Import path updated to use versioned package.The change from
lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/types"tolockuptypes "cosmossdk.io/x/accounts/defaults/lockup/v1"aligns with good versioning practices. This update suggests a transition to a versioned structure for thelockupmodule, which is beneficial for managing API changes and maintaining backward compatibility.To ensure consistency across the codebase, please run the following command:
This will help identify any files that might still be using the old import path.
Consider updating any relevant documentation (e.g., README files, API docs) to reflect this versioning change.
✅ Verification successful
Verified: All
lockuptypesimports updated to use thev1path.The verification confirms that all import statements for
lockuptypesin Go files now referencecosmossdk.io/x/accounts/defaults/lockup/v1, ensuring consistency across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all imports of lockuptypes use the new v1 path rg -g '*.go' 'lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/' --statsLength of output: 1370
tests/e2e/accounts/lockup/periodic_lockup_test_suite.go (1)
13-13: LGTM! Verify imported types remain unchanged.The import path change from "types" to "v1" aligns with the PR objectives and the broader restructuring of the
lockupmodule. This change reflects a shift towards versioned modules, which is a good practice for maintaining backwards compatibility.To ensure that this change doesn't introduce any unintended side effects, please run the following script to verify that the imported types remain unchanged:
x/accounts/defaults/lockup/periodic_locking_account.go (1)
Line range hint
1-385: LGTM. Ensure comprehensive testing with the new lockuptypes version.The code changes look good. The update to the
lockuptypesimport path doesn't seem to require any modifications to the existing code, which suggests backwards compatibility. However, it's crucial to ensure that all functionalities work as expected with the new version.Please run the following script to verify the functionality:
#!/bin/bash # Description: Run tests to ensure compatibility with the new lockuptypes version. # Test: Run all tests in the lockup package go test ./x/accounts/defaults/lockup/... # Test: Run tests for packages that depend on lockup go test ./x/accounts/...api/cosmos/accounts/defaults/lockup/v1/query.pulsar.go (7)
23-24: Updates align with package renamingThe changes to the initialization function and message descriptor are consistent with the package renaming to
lockupv1.
36-36: Confirm consistency in variable names after package renamingEnsure that all variable names updated with the
_v1suffix are consistent throughout the codebase. This helps maintain clarity and prevents potential conflicts.
642-643: Consistent updates to message descriptorsThe message descriptor initializations reflect the package renaming and are correctly updated.
663-663: Verify message state initializationEnsure that the message state initialization uses the correct index corresponding to the message types after the package renaming.
782-796: Check field descriptors for consistencyThe field descriptors within
Hasmethod are updated with the new package path. Confirm that all field names and paths are correctly updated throughout the codebase.
109-109:⚠️ Potential issueUpdate error messages to reflect package changes
The panic error messages still reference the old package path
cosmos.accounts.defaults.lockup.v1. Verify that this is intentional and consistent with the new package structure. If the package name changes as suggested, these messages should be updated accordingly.Also applies to: 125-125, 141-141, 161-161, 181-181, 194-194, 206-206
800-802:⚠️ Potential issueHandle extensions appropriately
The panic message indicates that proto3 declared messages do not support extensions for
cosmos.accounts.defaults.lockup.v1.QueryLockupAccountInfoResponse. Verify whether extensions are required or if this can be safely omitted.
|
|
||
| import "amino/amino.proto"; | ||
| import "cosmos/base/v1beta1/coin.proto"; | ||
| import "gogoproto/gogo.proto"; | ||
| import "google/protobuf/duration.proto"; | ||
|
|
||
| option go_package = "cosmossdk.io/x/accounts/defaults/lockup/types"; | ||
| option go_package = "cosmossdk.io/x/accounts/defaults/lockup/v1"; |
There was a problem hiding this comment.
💡 Codebase verification
Old package references detected in test files
The following test files still reference the old package cosmossdk.io/x/accounts/defaults/lockup/types:
tests/e2e/accounts/lockup/permanent_lockup_test_suite.gotests/e2e/accounts/lockup/continous_lockup_test_suite.gotests/e2e/accounts/lockup/delayed_lockup_test_suite.go
Please update these references to the new package path to ensure consistency.
🔗 Analysis chain
Verify go_package option consistency
The update to the go_package option correctly reflects the new versioned structure. This ensures that generated Go code will use the correct import path.
To ensure consistency across the codebase, please run the following command:
This will help identify any remaining references to the old package that might need updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all references to the old package have been updated
rg "cosmossdk.io/x/accounts/defaults/lockup/types" --type go
Length of output: 510
| @@ -8,7 +8,7 @@ import ( | |||
|
|
|||
| "cosmossdk.io/core/transaction" | |||
| "cosmossdk.io/simapp" | |||
| "cosmossdk.io/x/accounts/defaults/lockup/types" | |||
| types "cosmossdk.io/x/accounts/defaults/lockup/v1" | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent import paths for types package.
The import path for the types package varies between cosmossdk.io/x/accounts/defaults/lockup/v1 and cosmossdk.io/x/accounts/defaults/lockup/types across different files. To maintain consistency and prevent potential issues, please standardize all import paths to use either v1 or types consistently throughout the codebase.
Affected files:
- tests/e2e/accounts/lockup/continous_lockup_test_suite.go
- tests/e2e/accounts/lockup/permanent_lockup_test_suite.go
- tests/e2e/accounts/lockup/delayed_lockup_test_suite.go
🔗 Analysis chain
Import path updated correctly.
The import path for the types package has been updated from cosmossdk.io/x/accounts/defaults/lockup/types to cosmossdk.io/x/accounts/defaults/lockup/v1. This change aligns with the PR objective to fix the proto path and is consistent with similar changes in other files of this PR.
However, there's an ongoing discussion about whether to use "v1" or "types" in the import path. While "v1" is consistent with prior implementations, it might be worth considering using "types" for clarity if that's the preferred convention moving forward.
To ensure consistency across the codebase, let's verify the usage of this import path:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in import paths for lockup types
# Expected result: All imports should use the same path (either v1 or types)
echo "Checking import paths for lockup types:"
rg --type go 'cosmossdk\.io/x/accounts/defaults/lockup/(v1|types)'
Length of output: 1976
| @@ -6,7 +6,7 @@ import ( | |||
|
|
|||
| "cosmossdk.io/math" | |||
| "cosmossdk.io/x/accounts/accountstd" | |||
| lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/types" | |||
| lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/v1" | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Import Paths Detected
The old import path cosmossdk.io/x/accounts/defaults/lockup/types is still used in the following files:
tests/e2e/accounts/lockup/continous_lockup_test_suite.gotests/e2e/accounts/lockup/permanent_lockup_test_suite.gotests/e2e/accounts/lockup/delayed_lockup_test_suite.go
Please update these files to use cosmossdk.io/x/accounts/defaults/lockup/v1 to ensure consistency across the codebase.
🔗 Analysis chain
LGTM: Import path updated correctly.
The import path for lockuptypes has been updated from cosmossdk.io/x/accounts/defaults/lockup/types to cosmossdk.io/x/accounts/defaults/lockup/v1. This change aligns with the PR objectives and the broader restructuring of the lockup module to accommodate versioning.
To ensure consistency across the codebase, let's verify the usage of this new import path:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of the new import path across the codebase.
# Test: Search for the old import path. Expect: No results.
echo "Checking for old import path:"
rg "cosmossdk.io/x/accounts/defaults/lockup/types"
# Test: Search for the new import path. Expect: Consistent usage across relevant files.
echo "Checking for new import path:"
rg "cosmossdk.io/x/accounts/defaults/lockup/v1"
Length of output: 2443
| @@ -11,7 +11,7 @@ import ( | |||
| "cosmossdk.io/core/store" | |||
| "cosmossdk.io/log" | |||
| "cosmossdk.io/math" | |||
| lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/types" | |||
| lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/v1" | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Import Paths Detected
The following files still use the old import path cosmossdk.io/x/accounts/defaults/lockup/types and need to be updated to v1:
tests/e2e/accounts/lockup/permanent_lockup_test_suite.gotests/e2e/accounts/lockup/continous_lockup_test_suite.gotests/e2e/accounts/lockup/delayed_lockup_test_suite.go
🔗 Analysis chain
LGTM: Import path updated correctly.
The import path for lockuptypes has been updated from cosmossdk.io/x/accounts/defaults/lockup/types to cosmossdk.io/x/accounts/defaults/lockup/v1. This change is consistent with the PR objectives to address import naming inconsistency.
To ensure consistency across the codebase, please run the following command:
This will help identify any remaining instances of the old import path that may need updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of the new import path
rg -g '*.go' 'cosmossdk\.io/x/accounts/defaults/lockup/(types|v1)'
Length of output: 1887
| @@ -11,7 +11,7 @@ import ( | |||
| "cosmossdk.io/core/store" | |||
| "cosmossdk.io/log" | |||
| "cosmossdk.io/math" | |||
| lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/types" | |||
| lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/v1" | |||
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Update Remaining Import Paths for lockuptypes
The old import path cosmossdk.io/x/accounts/defaults/lockup/types is still present in the following test files:
tests/e2e/accounts/lockup/continous_lockup_test_suite.gotests/e2e/accounts/lockup/delayed_lockup_test_suite.gotests/e2e/accounts/lockup/permanent_lockup_test_suite.go
Please update these import statements to use the new versioned package structure.
🔗 Analysis chain
LGTM: Import path updated correctly.
The import path for lockuptypes has been updated to use the new versioned package structure. This change aligns with the PR objective and maintains consistency with prior implementations.
To ensure consistency across the codebase, please run the following command to check for any remaining occurrences of the old import path:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining occurrences of the old import path
rg "cosmossdk.io/x/accounts/defaults/lockup/types"
Length of output: 366
| @@ -9,7 +9,7 @@ import ( | |||
|
|
|||
| "cosmossdk.io/core/store" | |||
| "cosmossdk.io/math" | |||
| lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/types" | |||
| lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/v1" | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent import paths found for lockuptypes
The following files are still using the old import path cosmossdk.io/x/accounts/defaults/lockup/types or cosmossdk.io/x/accounts/defaults/lockup/depinject instead of the updated v1 path:
tests/e2e/accounts/lockup/delayed_lockup_test_suite.gotests/e2e/accounts/lockup/permanent_lockup_test_suite.gotests/e2e/accounts/lockup/continous_lockup_test_suite.gosimapp/v2/app_di.gosimapp/app_di.go
Please update these import statements to ensure consistency across the codebase.
🔗 Analysis chain
LGTM: Import path updated to use versioned package.
The change from lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/types" to lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/v1" aligns with the PR objective of addressing import naming inconsistency. This shift to a versioned package structure is a good practice for managing API changes.
To ensure consistency across the codebase, please run the following command:
This command will search for any remaining imports that don't use the new v1 path. The results should be empty if the change has been applied consistently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all imports of lockuptypes use the new v1 path
rg -g '*.go' 'cosmossdk\.io/x/accounts/defaults/lockup/(?!v1)' --stats
Length of output: 364
Script:
#!/bin/bash
# Find all .go files with imports of lockuptypes not using the v1 path
# Extract lines with the lockup import path
rg 'cosmossdk\.io/x/accounts/defaults/lockup/' -g '*.go' > all_lockup_imports.txt
# Exclude the v1 path
grep -v 'lockup/v1' all_lockup_imports.txt > invalid_imports.txt
# Output the result
if [ -s invalid_imports.txt ]; then
echo "Found imports not using the v1 path:"
cat invalid_imports.txt
else
echo "All lockuptypes imports use the v1 path."
fi
Length of output: 734
| @@ -11,7 +11,7 @@ import ( | |||
| "cosmossdk.io/core/store" | |||
| "cosmossdk.io/log" | |||
| "cosmossdk.io/math" | |||
| lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/types" | |||
| lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/v1" | |||
There was a problem hiding this comment.
💡 Codebase verification
Remaining Old Import Paths Found in Test Files
The following test files still import cosmossdk.io/x/accounts/defaults/lockup/types. Please update these import paths to the new versioned path (v1):
tests/e2e/accounts/lockup/permanent_lockup_test_suite.gotests/e2e/accounts/lockup/continous_lockup_test_suite.gotests/e2e/accounts/lockup/delayed_lockup_test_suite.go
🔗 Analysis chain
LGTM: Import path updated correctly.
The import path for lockuptypes has been updated to use the new versioned path (v1). This change is consistent with the PR objective and doesn't affect the functionality of the tests.
To ensure consistency across the codebase, please run the following command to check for any remaining occurrences of the old import path:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining occurrences of the old import path
rg "cosmossdk.io/x/accounts/defaults/lockup/types" --type go
Length of output: 376
| @@ -9,7 +9,7 @@ import ( | |||
| errorsmod "cosmossdk.io/errors" | |||
| "cosmossdk.io/math" | |||
| "cosmossdk.io/x/accounts/accountstd" | |||
| lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/types" | |||
| lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/v1" | |||
There was a problem hiding this comment.
💡 Codebase verification
Old lockuptypes import paths still present in tests:
tests/e2e/accounts/lockup/continous_lockup_test_suite.gotests/e2e/accounts/lockup/permanent_lockup_test_suite.gotests/e2e/accounts/lockup/delayed_lockup_test_suite.go
🔗 Analysis chain
Verify the impact of the lockuptypes import change.
The import path for lockuptypes has been updated to use v1. This change might affect the usage of lockuptypes throughout the file. Please ensure that all references to lockuptypes are compatible with the new version.
To verify the impact, run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any incompatibilities or changes required due to the new lockuptypes import.
# Test: Search for all usages of lockuptypes in the file
rg 'lockuptypes\.' x/accounts/defaults/lockup/periodic_locking_account.go
# Test: Check if there are any compilation errors due to the import change
go build ./x/accounts/defaults/lockup/...
Length of output: 295
Script:
#!/bin/bash
# Description: Verify that the lockuptypes import path has been updated consistently and no old references remain.
# Test 1: Ensure the old import path does not exist anywhere in the codebase
echo "Checking for old import path 'cosmossdk.io/x/accounts/defaults/lockup/types'..."
rg 'cosmossdk.io/x/accounts/defaults/lockup/types' || echo "No occurrences of the old import path found."
# Test 2: Ensure the new import path is used consistently
echo "Checking for new import path 'cosmossdk.io/x/accounts/defaults/lockup/v1'..."
rg 'cosmossdk.io/x/accounts/defaults/lockup/v1' || echo "No occurrences of the new import path found."
# Test 3: Find all usages of lockuptypes to verify compatibility
echo "Finding all usages of 'lockuptypes'..."
rg 'lockuptypes\.'
Length of output: 16958
| @@ -1,5 +1,5 @@ | |||
| // Code generated by protoc-gen-go-pulsar. DO NOT EDIT. | |||
| package lockup | |||
| package lockupv1 | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing the version number from the package name
Including version numbers in package names is generally discouraged in Go. Package names should be concise, all lowercase, and without underscores or version numbers. Versioning is typically managed through Go modules rather than package names.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| lv := value.List() | ||
| clv := lv.(*_Period_2_list) | ||
| x.Amount = *clv.list | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.Period")) | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.v1.Period")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| } | ||
| panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.Period does not contain field %s", fd.FullName())) | ||
| panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.v1.Period does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| @@ -158,9 +158,9 @@ | |||
| switch fd.FullName() { | |||
| default: | |||
| if fd.IsExtension() { | |||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.QueryLockupAccountInfoRequest")) | |||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.v1.QueryLockupAccountInfoRequest")) | |||
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| } | ||
| panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.QueryLockupAccountInfoRequest does not contain field %s", fd.FullName())) | ||
| panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.v1.QueryLockupAccountInfoRequest does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| x.Owner = value.Interface().(string) | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.QueryLockupAccountInfoResponse")) | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.v1.QueryLockupAccountInfoResponse")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| } | ||
| panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.QueryLockupAccountInfoResponse does not contain field %s", fd.FullName())) | ||
| panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.v1.QueryLockupAccountInfoResponse does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| @@ -1782,9 +1782,9 @@ | |||
| switch fd.FullName() { | |||
| default: | |||
| if fd.IsExtension() { | |||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.QueryLockingPeriodsRequest")) | |||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.v1.QueryLockingPeriodsRequest")) | |||
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| } | ||
| panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.QueryLockingPeriodsRequest does not contain field %s", fd.FullName())) | ||
| panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.v1.QueryLockingPeriodsRequest does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| lv := value.List() | ||
| clv := lv.(*_QueryLockingPeriodsResponse_1_list) | ||
| x.LockingPeriods = *clv.list | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.QueryLockingPeriodsResponse")) | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.v1.QueryLockingPeriodsResponse")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| } | ||
| panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.QueryLockingPeriodsResponse does not contain field %s", fd.FullName())) | ||
| panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.v1.QueryLockingPeriodsResponse does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 2711ece) # Conflicts: # api/cosmos/accounts/defaults/lockup/v1/lockup.pulsar.go # api/cosmos/accounts/defaults/lockup/v1/query.pulsar.go # api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go
Description
noticed while reviewing #22254
import naming inconsistency between all account impls
Shall we use v1? or types? I'd actually suggest the later, as v1 feels weird, but this is what was done previously.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
lockupv1.Bug Fixes
Documentation
Chores