chore: update tagged submodules for v27 #8801
Conversation
crnbarr93
added
V:state/breaking
crnbarr93
requested review from
mattverse,
nicolaslara and
p0mvn
as code owners
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)x/tokenfactory/keeper/before_send_test.go (1)
Verify the change in error expectation for minting 100 tokens. The test expectation has been changed to expect no error when minting 100 tokens of the factory denomination. This is a significant behavioral change that needs verification:
Let's verify if this change is consistent with the module's behavior: Please confirm:
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 (
|
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested labels
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)
x/tokenfactory/keeper/bankactions.go (1)
91-97: Consider standardizing error messages for consistency.While the validation logic is correct, the error messages could be more consistent:
- Line 91: "send from module acc not available"
- Line 96: "send to module acc not available"
Consider standardizing the messages:
- return status.Errorf(codes.Internal, "send from module acc not available") + return status.Errorf(codes.Internal, "sending from module account is not allowed") - return status.Errorf(codes.Internal, "send to module acc not available") + return status.Errorf(codes.Internal, "sending to module account is not allowed")x/tokenfactory/keeper/keeper.go (1)
Line range hint
19-29: LGTM! Consider adding documentation for permAddrMapThe addition of
permAddrMapis a good optimization for permission lookups. However, it would be helpful to add documentation explaining its purpose and relationship withpermAddrs.Add a comment above the Keeper struct or the field:
type ( Keeper struct { storeKey storetypes.StoreKey permAddrs map[string]authtypes.PermissionsForAddress + // permAddrMap provides O(1) lookup for checking if an address has any permissions + // This is an optimization to avoid iterating through permAddrs permAddrMap map[string]boolCHANGELOG.md (1)
53-53: LGTM with suggestion to add more details.The changelog entry accurately reflects that this PR updates tagged submodules for v27. However, consider adding more details about the specific submodule versions being updated:
-* [#8801](https://github.com/osmosis-labs/osmosis/pull/8801) chore: update tagged submodules for v27 +* [#8801](https://github.com/osmosis-labs/osmosis/pull/8801) chore: update tagged submodules for v27: + * x/ibc-hooks: v0.0.17 + * x/epochs: v0.0.11 + * osmoutils: v0.0.15 + * osmomath: v0.0.15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumgo.workis excluded by!**/*.work,!**/*.work
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)x/tokenfactory/keeper/bankactions.go(2 hunks)x/tokenfactory/keeper/keeper.go(1 hunks)
🔇 Additional comments (5)
x/tokenfactory/keeper/bankactions.go (2)
69-74: LGTM! Improved variable naming and error handling.
The variable renaming from fromAddr to fromSdkAddr better indicates the type, and early initialization of toSdkAddr follows good error handling practices.
91-97: Verify similar validation patterns across the codebase.
Let's ensure this validation pattern is consistently applied across other token transfer functions in the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
Module account validation is unique to tokenfactory's bankactions
The validation pattern for preventing transfers to/from module accounts appears to be specific to the tokenfactory module's bankactions. While there are numerous instances of SendCoinsFromModuleToAccount and similar bank operations across the codebase, none of them implement the same validation pattern. This is expected as different modules have different requirements for module account interactions.
- The tokenfactory module's strict validation is intentional and doesn't need to be replicated elsewhere
- Other modules like superfluid, mint, and lockup handle module account transfers differently based on their specific use cases
- No inconsistencies found in the validation patterns across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar token transfer functions and their validation patterns
rg -A 5 "SendCoins.*ModuleName" --type go
# Look for other module account validation patterns
ast-grep --pattern 'if $_.GetAddress().Equals($_) {
return status.Errorf($_)
}'
Length of output: 10656
x/tokenfactory/keeper/keeper.go (3)
Line range hint 42-52: LGTM! Initialization is correct and consistent
The initialization of permAddrMap is properly synchronized with permAddrs, ensuring consistency in permission tracking.
57-60: LGTM! Proper initialization of all fields
The Keeper struct initialization correctly includes the new permAddrMap field while maintaining proper alignment and structure.
Line range hint 19-29: Verify the usage of permAddrMap across the codebase
Let's ensure this new field is being utilized effectively in the codebase.
✅ Verification successful
Let me gather more context about the permission checking methods to ensure we have a complete picture.
Let me try one more search pattern to find all permission-related functionality.
permAddrMap is properly implemented and used for module account verification
The new permAddrMap field is being used effectively in the IsModuleAcc method to efficiently check if an address is a restricted module account. The implementation shows:
- Initialization in the keeper constructor where it's populated from module permissions
- Used in
IsModuleAccmethod for O(1) lookups of restricted addresses - Serves as a performance optimization over iterating through
permAddrs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential usages of the new permAddrMap field
rg -A 3 "permAddrMap" ./x/tokenfactory/
# Look for any permission checking methods that might need to be updated
ast-grep --pattern 'func ($receiver *Keeper) $method($ctx sdk.Context, $addr string) bool {
$$$
}'
Length of output: 1282
Script:
#!/bin/bash
# Search for methods that check permissions in bankactions.go
rg -A 5 "func \(\*Keeper\) .*Permission" ./x/tokenfactory/
# Look for any other methods that might be using the permAddrMap
ast-grep --pattern 'func ($receiver *Keeper) $method($params) $return {
$$$
$receiver.permAddrMap
$$$
}'
Length of output: 173
Script:
#!/bin/bash
# Search for any permission-related code and methods
rg -A 5 "HasPermission|CheckPermission|IsPermission" ./x/tokenfactory/
# Also look for the specific method using permAddrMap we found earlier
rg -A 10 -B 5 "return k.permAddrMap\[addr.String\(\)\]" ./x/tokenfactory/
Length of output: 673
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
x/tokenfactory/keeper/keeper.go (2)
Line range hint
19-31: LGTM! Good performance optimization.The addition of
permAddrMapas a boolean lookup map is a good optimization for permission checks. It provides O(1) lookup time and avoids repeated address string conversions that would be needed when using only thepermAddrsmap.Consider documenting the relationship between
permAddrsandpermAddrMapin comments, as they serve complementary purposes:
permAddrs: Stores detailed permissionspermAddrMap: Provides fast lookup for permission checks
57-60: Consider pre-allocating map capacity.The initialization and population of
permAddrMapis correct, but we can optimize it further by pre-allocating the map capacity since we know the size frommaccPerms.- permAddrMap := make(map[string]bool) + permAddrMap := make(map[string]bool, len(maccPerms))CHANGELOG.md (1)
53-53: Consider documenting additional v27 changes.The PR objectives indicate this is for v27 but currently only submodule updates are documented. Consider adding entries for any other changes that will be included in the v27 release.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumgo.workis excluded by!**/*.work,!**/*.work
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)x/tokenfactory/keeper/bankactions.go(2 hunks)x/tokenfactory/keeper/keeper.go(1 hunks)
🔇 Additional comments (4)
x/tokenfactory/keeper/bankactions.go (3)
69-74: LGTM! Improved variable naming and consistent error handling.
The variable renaming from fromAcc to fromSdkAddr and the introduction of toSdkAddr enhance code clarity by explicitly indicating the SDK address type. The error handling is consistent for both address conversions.
Line range hint 69-97: LGTM! Solid implementation with proper validations.
The overall implementation is well-structured with:
- Proper validation of token factory denominations
- Consistent error handling for address conversions
- Deterministic processing through sorted iteration of module accounts
- Clear separation of validation steps
91-97: Verify impact of new module account transfer restriction.
While the added validation for preventing transfers to module accounts is good, we should verify this doesn't break existing functionality. Also, consider making the error messages consistent between the "from" and "to" validations.
Let's verify the module account transfer usage:
Consider making the error messages consistent:
- return status.Errorf(codes.Internal, "send from module acc not available")
+ return status.Errorf(codes.Internal, "transfer from module account not allowed")
- return status.Errorf(codes.Internal, "send to module acc not available")
+ return status.Errorf(codes.Internal, "transfer to module account not allowed")✅ Verification successful
Based on the search results, I can now generate the final response:
Module account transfer restriction is correctly implemented
The added validation preventing transfers to module accounts is safe and doesn't break existing functionality because:
- All existing module account transfers in the codebase use specialized methods like
SendCoinsFromModuleToAccountandSendCoinsFromAccountToModule - The new validation in
bankactions.goonly affects the tokenfactory's force transfer functionality - Test coverage (
TestForceTransfer) exists and can verify this behavior
The error message consistency suggestion remains valid:
- return status.Errorf(codes.Internal, "send from module acc not available")
+ return status.Errorf(codes.Internal, "transfer from module account not allowed")
- return status.Errorf(codes.Internal, "send to module acc not available")
+ return status.Errorf(codes.Internal, "transfer to module account not allowed")🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing tokenfactory transfer calls to module accounts
rg -A 5 'forceTransfer.*ModuleName|SendCoins.*ModuleName' --type go
# Search for tests that might be affected
rg -A 5 'TestForceTransfer' --type go
Length of output: 11483
CHANGELOG.md (1)
53-53: LGTM on the submodule updates for v27.
The submodule version updates look correct and match the PR objectives:
- x/ibc-hooks: v0.0.17
- x/epochs: v0.0.11
- osmoutils: v0.0.15
- osmomath: v0.0.15
* chore: update tagged submodules for v27 * chore: changelog * test: fix before_send_test TestBeforeSendHook (cherry picked from commit 9e46748)
* chore: update tagged submodules for v27 * chore: changelog * test: fix before_send_test TestBeforeSendHook (cherry picked from commit 9e46748) Co-authored-by: Connor Barr <[email protected]>
What is the purpose of the change
Updating the node to tagged submodules
x/ibc-hooks ⇒ https://github.com/osmosis-labs/osmosis/releases/tag/x%2Fibc-hooks%2Fv0.0.17
x/epochs ⇒ https://github.com/osmosis-labs/osmosis/releases/tag/x%2Fepochs%2Fv0.0.11
osmoutils ⇒ https://github.com/osmosis-labs/osmosis/releases/tag/osmoutils%2Fv0.0.15
osmomath ⇒ https://github.com/osmosis-labs/osmosis/releases/tag/osmomath%2Fv0.0.15