feat: upgrade + audit fixes #349
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a series of updates across various components. Changes include dependency version updates in the Dockerfile, integration of new Block SDK features in the application, refactoring of keeper initializations, modifications in upgrade logic, and enhanced validation in protocol buffer definitions. Additionally, the PR refactors contract building logic and alters test flows, while updating naming conventions and control flows in middleware and DEX-related code. Changes
Sequence Diagram(s)sequenceDiagram
participant App as InitiaApp
participant GRPC as GRPCGatewayRouter
participant Block as Block SDK Services
App->>GRPC: Register API Routes
GRPC->>Block: blockservice.RegisterGRPCGatewayRoutes
App->>Block: blockservice.RegisterMempoolService
sequenceDiagram
participant CLI as Deploy Command
participant Build as buildContract Function
participant API as api.BuildContract
participant FS as File System
CLI->>Build: Invoke buildContract(arg)
Build->>FS: Get current working directory
Build->>API: Call api.BuildContract(arg)
API-->>Build: Return result/error
Build->>FS: Check & reset directory if necessary
Build-->>CLI: Return final error/status
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 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 (
|
Dependency ReviewThe following issues were found:
License Issuesgo.mod
Denied Licenses: GPL-1.0-or-later, LGPL-2.0-or-later OpenSSF Scorecard
Scanned Files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/upgrade.go (1)
17-17: Module migrations are now executed as part of the upgrade process.The upgrade handler now returns the result of
app.ModuleManager.RunMigrations()instead of directly returning the version map. This is a significant change that runs module migrations during the upgrade process, ensuring that any necessary state migrations occur automatically.Consider adding logging or telemetry to track migration execution success/failure for better observability during upgrades.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
client/docs/config.jsonis excluded by!**/*.jsonclient/docs/swagger-ui/swagger.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (14)
Dockerfile(1 hunks)app/app.go(3 hunks)app/keepers/keepers.go(0 hunks)app/upgrade.go(1 hunks)cmd/move/deploy.go(3 hunks)cmd/move/move.go(1 hunks)scripts/protoc-swagger-gen.sh(3 hunks)x/distribution/keeper/common_test.go(1 hunks)x/distribution/keeper/delegation_test.go(1 hunks)x/ibc/nft-transfer/types/packet.go(2 hunks)x/intertx/keeper/msg_server.go(1 hunks)x/move/keeper/balancer.go(1 hunks)x/move/keeper/balancer_test.go(2 hunks)x/move/types/connector.go(1 hunks)
💤 Files with no reviewable changes (1)
- app/keepers/keepers.go
✅ Files skipped from review due to trivial changes (2)
- x/distribution/keeper/delegation_test.go
- x/distribution/keeper/common_test.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: golangci-lint
- GitHub Check: Run test and upload codecov
- GitHub Check: Initiad
- GitHub Check: Analyze (go)
🔇 Additional comments (22)
app/upgrade.go (1)
10-10:❓ Verification inconclusive
Verify version compatibility with dependencies.
The constant
upgradeNamehas been updated from "0.7.0" to "0.7.3". This version bump should align with the upgraded dependencies throughout the project, such as theLIBMOVEVM_VERSIONin the Dockerfile.
🏁 Script executed:
#!/bin/bash # Verify dependency versions match the upgrade version echo "Checking for version references in project..." rg -e "0\.7\.3|0\.7\.0" --type goLength of output: 179
Action Required: Confirm Dependency Version Alignment
The constantupgradeNameinapp/upgrade.gohas been correctly updated to"0.7.3". However, the current version-check script only confirms this change in Go files. There’s no verification that other dependencies of the project (for instance, theLIBMOVEVM_VERSIONsetting in the Dockerfile) have been updated accordingly. Please double-check that any dependency version references—especially in non-Go files like the Dockerfile—are aligned with this bump.x/move/types/connector.go (1)
59-59:✅ Verification successful
Function naming change affects DEX swap execution method.
The constant has been renamed from
FunctionNameDexSwapScripttoFunctionNameDexSudoSwap, suggesting a change from a script-based swap to a sudo (privileged) operation. This aligns with the changes inbalancer_test.gothat now use the fee collector address.
🏁 Script executed:
#!/bin/bash # Find references to the renamed function to ensure consistent updates echo "Checking for references to the old and new function names..." rg -e "FunctionNameDexSwapScript|FunctionNameDexSudoSwap" --type goLength of output: 324
DEX Swap Execution Naming Updated – Verified
The constant is now correctly renamed from
FunctionNameDexSwapScripttoFunctionNameDexSudoSwapto reflect the move towards a sudo-based execution method. We verified this change with a search that confirms consistent usage in bothx/move/types/connector.goandx/move/keeper/balancer.go. No further modifications are required.Dockerfile (1)
9-9:✅ Verification successful
Library version upgrade from v0.6.1 to v0.7.0.
The
LIBMOVEVM_VERSIONenvironment variable has been updated to v0.7.0, which will cause the build process to download and use newer libraries. This aligns with the application upgrade from 0.7.0 to 0.7.3.
🏁 Script executed:
#!/bin/bash # Check if the version is consistent across the codebase and if there are any compatibility notes echo "Checking for libmovevm version references..." rg -e "v0\.6\.1|v0\.7\.0" --type go --type dockerfile # Check release notes or changelog if available curl -s "https://github.com/initia-labs/movevm/releases/tag/v0.7.0" | grep -A 10 "Release v0.7.0"Length of output: 1774
Library version upgrade verified – Ready to merge
- The Dockerfile now correctly sets
LIBMOVEVM_VERSION=v0.7.0(line 9), ensuring alignment with the application upgrade from 0.7.0 to 0.7.3.- A review of the release notes confirms the update to v0.7.0.
- No lingering references to v0.6.1 were observed in the inspected contexts.
x/move/keeper/balancer_test.go (2)
148-153: Test updated to use fee collector for swap operations.The test now transfers coins to the fee collector address before calling
SwapToBase, reflecting a change in theSwapToBaseimplementation that likely now requires privileged access or uses the fee collector module as part of the swap process.This change is consistent with the renaming of
FunctionNameDexSwapScripttoFunctionNameDexSudoSwapin the connector.go file.
156-156: Updated assertion to check fee collector's balance.The test now correctly checks the fee collector's balance after the swap operation, consistent with the earlier change that uses the fee collector for the swap.
x/intertx/keeper/msg_server.go (1)
65-66:❓ Verification inconclusive
Timeout value is now fixed rather than dynamic.
The code has been changed to use a fixed 5-minute relative timeout instead of what was likely a dynamic timeout based on block time. This simplifies the code but make sure this duration is sufficient for all expected transaction scenarios.
You may want to confirm if this timeout aligns with other timeout settings in the application:
🏁 Script executed:
#!/bin/bash # Description: Search for similar timeout settings in the codebase to ensure consistency rg -A 2 -B 2 "timeout.*time.Minute" --type goLength of output: 47
Action Required: Verify Fixed Timeout Duration Consistency
The code now uses a fixed 5‑minute timeout at
x/intertx/keeper/msg_server.go(lines 65–66):relativeTimeoutTimestamp := uint64((time.Minute * 5).Nanoseconds()) icaMsg := icacontrollertypes.NewMsgSendTx(msg.Owner, msg.ConnectionId, uint64(relativeTimeoutTimestamp), packetData)Automated searches for similar timeout configurations (using
rg "timeout.*time.Minute") didn’t return any output. This makes it unclear whether this fixed value aligns with timeout settings elsewhere in the application. Please manually verify that a 5‑minute duration is appropriate and consistent with other timeout expectations within the project.x/move/keeper/balancer.go (1)
61-70:❓ Verification inconclusive
Function signature and parameter changes in swap implementation.
The swap implementation has been updated with several changes:
- Using
executeEntryFunctioninstead ofExecuteEntryFunction(changed from exported to unexported)- Passing addresses as a slice with
StdAddressfirst- Changed function name from
FunctionNameDexSwapScripttoFunctionNameDexSudoSwap- Added a new boolean parameter
Ensure that all calling code has been updated to account for these changes, particularly if
ExecuteEntryFunctionwas previously used externally.
🏁 Script executed:
#!/bin/bash # Description: Check if ExecuteEntryFunction is still used elsewhere and verify DexSwapScript references # Check for any remaining uses of ExecuteEntryFunction rg "ExecuteEntryFunction" --type go # Check for any remaining references to DexSwapScript rg "DexSwapScript" --type goLength of output: 6200
Update Dex Swap Implementation – Verify External Callers
The dex swap implementation in
x/move/keeper/balancer.gohas been modified as follows:
- Changed Function Access: Now calls the unexported
executeEntryFunctioninstead of the exportedExecuteEntryFunction.- Parameter Order Updated: Passing the addresses as a slice with
vmtypes.StdAddressfirst.- Function Name Update: Uses
FunctionNameDexSudoSwapinstead ofFunctionNameDexSwapScript(and our search confirms no remaining references to the old name).- New Boolean Parameter: A new flag has been added to the call.
However, our repository search still shows several external references to
ExecuteEntryFunction(in tests and other modules). Please verify that these callers are either unaffected by—or appropriately updated for—the change, ensuring there is no inconsistency between internal and external usage.// Updated code snippet in x/move/keeper/balancer.go (lines 61-70) return k.executeEntryFunction( ctx, []vmtypes.AccountAddress{vmtypes.StdAddress, trader}, vmtypes.StdAddress, types.MoveModuleNameDex, types.FunctionNameDexSudoSwap, []vmtypes.TypeTag{}, [][]byte{metadataLP[:], metadataQuote[:], offerAmountBz, {0}}, false, )If external callers (or tests) still rely on
ExecuteEntryFunction, please confirm that they remain valid or adjust them to ensure consistency with the new implementation.app/app.go (3)
74-75: New Block SDK integration.Adding imports for the Block SDK modules, which aligns with the PR objectives of implementing dependency upgrades.
454-455: New Block SDK API routes registration.Registering the Block SDK mempool API routes with the gRPC Gateway router. This is part of the Block SDK integration.
478-484: Additional Block SDK mempool service registration with type safety.The code now registers the Block SDK mempool transaction service and correctly ensures type safety by checking if the mempool implements the required interface. This follows good defensive programming practices.
cmd/move/deploy.go (4)
155-156: LGTM!The formatting change improves code readability by placing the closing brace on a new line.
45-45: LGTM!Refactoring to use the new
buildContracthelper function improves code organization.
158-158: LGTM!Consistent usage of the new helper function across similar code paths.
172-192: Good refactoring to handle working directory changes.This new helper function centralizes contract building logic and properly handles working directory changes that might occur during the build process.
The function correctly:
- Saves the current working directory
- Calls the build API
- Checks if the working directory changed
- Restores the original directory if needed
This prevents potential bugs where directory changes during the build process might affect subsequent operations.
scripts/protoc-swagger-gen.sh (4)
14-15: LGTM!Properly added new variables for Block SDK integration.
22-22: LGTM!Correctly extracts the Block SDK version from go.mod file using the same pattern as other dependencies.
31-31: LGTM!Added git clone command for Block SDK repository at the specified version.
45-45: LGTM!Added Block SDK proto files path to the proto_dirs variable for Swagger generation.
x/ibc/nft-transfer/types/packet.go (4)
55-55: Improved validation for ClassId.Changed validation to use
strings.TrimSpace()which is more robust than simply checking length, as it properly handles whitespace-only strings.
61-66: Enhanced validation for TokenUris and TokenData.The validation logic has been improved to be more flexible, allowing either:
- Empty collections (length 0)
- Collections with the same length as TokenIds
This change supports more use cases while maintaining data integrity.
69-69: Improved tokenId validation.Using
strings.TrimSpace()to check for blank tokenIds is more robust than checking only for zero length, as it properly handles whitespace-only strings.
125-132: Good data handling improvement.This change ensures TokenUris and TokenData are properly initialized with empty strings instead of nil values when decoded, making the data structure more consistent and preventing potential nil pointer issues in downstream code.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #349 +/- ##
==========================================
+ Coverage 41.18% 41.21% +0.02%
==========================================
Files 269 269
Lines 25769 25761 -8
==========================================
+ Hits 10613 10617 +4
+ Misses 13507 13498 -9
+ Partials 1649 1646 -3
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
x/ibc-hooks/types/types.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/move/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/mstaking/types/authz.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (6)
api/initia/ibchooks/v1/types.pulsar.go(1 hunks)api/initia/move/v1/tx.pulsar.go(1 hunks)api/initia/mstaking/v1/authz.pulsar.go(2 hunks)proto/initia/ibchooks/v1/types.proto(1 hunks)proto/initia/move/v1/tx.proto(1 hunks)proto/initia/mstaking/v1/authz.proto(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: golangci-lint
- GitHub Check: Run test and upload codecov
- GitHub Check: Initiad
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
proto/initia/ibchooks/v1/types.proto (1)
13-13: Consistent naming convention applied in amino.name option.The change updates the amino.name from "ibc-hooks/Params" to "ibchooks/Params", aligning it with the module's naming structure. This ensures consistent naming patterns across the codebase.
api/initia/mstaking/v1/authz.pulsar.go (1)
1481-1533: Autogenerated changes to protobuf descriptors.These changes to the raw descriptor byte arrays reflect updates to the underlying protocol buffer definitions, specifically the StakeAuthorization message. Since this is an auto-generated file, these changes are expected as a result of Proto file modifications.
api/initia/ibchooks/v1/types.pulsar.go (1)
1004-1027: Autogenerated changes to protobuf descriptors.These modifications to the raw descriptor byte arrays reflect the changes made to the Params message in the proto/initia/ibchooks/v1/types.proto file, specifically the amino.name option update. These changes are automatically generated and shouldn't be manually edited.
api/initia/move/v1/tx.pulsar.go (1)
15554-15657: Protocol buffer file has been updated correctly.This file contains machine-generated code from protocol buffer definitions. The changes reflect updates to the underlying
.protodefinition file, including:
- Size adjustments for message types (changing from
0x9a, 0x01to0x97, 0x01and0x9a, 0x01to0xad, 0x01)- Updates to the
MsgDelistmessage- Addition or modification of service RPC method definitions
These changes are consistent with what would be expected from protocol buffer generation and align with the PR objective to update dependencies.
proto/initia/mstaking/v1/authz.proto (3)
14-14: Consistent Naming for Amino Option
The change from"mstake/StakeAuthorization"to"mstaking/StakeAuthorization"ensures consistency with the new naming convention. This update is clear and aligns with the audit fixes intended in the PR.
27-27: Update Amino Oneof Option for AllowList
The updated option from"mstake/StakeAuthorization/AllowList"to"mstaking/StakeAuthorization/AllowList"correctly applies the revised naming standard. Please verify that any components referencing this option are updated accordingly.
29-29: Update Amino Oneof Option for DenyList
The change from"mstake/StakeAuthorization/DenyList"to"mstaking/StakeAuthorization/DenyList"further reinforces the updated naming convention. This ensures uniformity in protocol buffer definitions across the module.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/move/keeper/staking.go (2)
388-390: Consider renaming themetadatasslice to something more descriptive.
While the usage ofmetadatasis functionally correct, a more explicit naming (e.g.,stakingMetadataAddresses) could help with code readability and clarity.
391-430: Evaluate the stop-on-error vs. partial iteration trade-off.
Currently, if an error occurs withinWalk, the code terminates the iteration by returningtrue, err. This prevents any subsequent staking entries from being processed. For slashing scenarios, you might consider whether partial iteration should continue (logging errors instead) to ensure maximum coverage of slashing operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/move/keeper/staking.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: golangci-lint
- GitHub Check: Run test and upload codecov
- GitHub Check: Initiad
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
x/move/keeper/staking.go (1)
382-383: BCS validator address serialization looks correct.
The usage ofvmtypes.SerializeString(valAddr.String())appears valid for applying a standardized representation of the validator address as a key.
Description
This PR includes dependency upgrades and also includes audit fixes.
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 changeReviewers 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.
I have...