feat: upgrades + audit fixes #97
Conversation
WalkthroughThis pull request integrates new Block SDK functionalities into the application. It registers Block SDK mempool API routes and transaction services in the core application, adds a new genesis parameter ( Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (2)
🪧 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
🧹 Nitpick comments (4)
scripts/protoc-swagger-gen.sh (2)
15-16: Adding Block SDK VariablesThe new environment variables for the Block SDK dependency (
BLOCK_SDK_URLandBLOCK_SDK_V) are defined similarly to other dependencies in the script. To improve robustness, consider quoting these values (e.g.,BLOCK_SDK_URL="github.com/skip-mev/block-sdk") to mitigate potential issues with special characters.
36-36: Cloning the Block SDK RepositoryThe git clone command for the Block SDK is correctly added, using the version extracted earlier. However, consider adding a check to ensure that
BLOCK_SDK_VERSIONis not empty before attempting the clone. This would help catch issues where the version extraction might fail and prevent runtime errors.client/docs/swagger-ui/swagger.yaml (1)
75707-75842: Review of API Model Definitions for SDK Auction and Mempool Responses
The definitions forsdk.auction.v1.Params,sdk.auction.v1.QueryParamsResponse, andsdk.mempool.v1.GetTxDistributionResponseare thoroughly described with clear property delineations and consistent type annotations (e.g., usage ofintegerwithformat: int64,stringwithformat: byte, etc.). A couple of suggestions for improvement:
- Potential Refactor Opportunity:
The schema for auction parameters (properties likemax_bundle_size,escrow_account_address,reserve_fee, etc.) appears in both the endpoint response and in the dedicated model definitions. If your Swagger/OpenAPI spec permits, consider refactoring to use a common definition via a$refto reduce duplication and ease future maintenance.- Verification of Multiline Descriptions:
Confirm that the use of the>-syntax for multiline descriptions renders as intended in Swagger UI.go.mod (1)
292-295: Temporary Replace Directive for cosmos/iavlA temporary replace directive has been added to override
github.com/cosmos/iavlwithgithub.jparrowsec.cn/initia-labs/iavl v0.0.0-20250223141407-caf697dd4712to address a statesync problem (referencing cosmos-sdk/issues/23740). It is recommended to document this workaround within the project documentation and track its removal once an upstream fix is available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
app/app.go(3 hunks)app/genesis.go(1 hunks)app/keepers/keepers.go(0 hunks)app/upgrade.go(2 hunks)client/docs/config.json(1 hunks)client/docs/swagger-ui/swagger.yaml(2 hunks)go.mod(4 hunks)scripts/protoc-swagger-gen.sh(4 hunks)
💤 Files with no reviewable changes (1)
- app/keepers/keepers.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: golangci-lint
- GitHub Check: Run test and upload codecov
- GitHub Check: Minitiad
🔇 Additional comments (17)
app/genesis.go (1)
123-123:✅ Verification successful
New parameter for gas limit added.
This introduces a
HookMaxGasparameter with a value of 3,000,000 units. This parameter likely sets the maximum gas limit for hook execution in the opchild module.
🏁 Script executed:
#!/bin/bash # Verify the HookMaxGas parameter value across the codebase for consistency rg -A 1 'DefaultHookMaxGas' --type goLength of output: 123
HookMaxGas Parameter Consistency Confirmed
The new parameter
HookMaxGasis now set to3,000,000in the genesis state (file: app/genesis.go, line 123) and is consistent with its usage elsewhere (e.g., app/upgrade.go assigns it usingopchildtypes.DefaultHookMaxGas). The verification shows that the default gas limit constant is referenced correctly across the codebase.app/upgrade.go (2)
12-12: Version updated to 0.7.1.The upgrade name has been updated from 0.6.4 to 0.7.1, which aligns with the dependency upgrades mentioned in the PR objectives.
34-34: Modified upgrade handler to run migrations.The upgrade handler now properly executes module migrations instead of simply returning the version map. This ensures that all module-specific upgrade handlers are executed during the upgrade process.
client/docs/config.json (1)
198-208: Added Block SDK API documentation.Two new API endpoints have been added to the Swagger configuration:
- Auction queries with parameter renaming for clarity
- Mempool queries for the Block SDK
This addition extends the API documentation to include the new Block SDK functionalities mentioned in the PR summary.
app/app.go (3)
73-74: Added Block SDK imports.These imports support the integration of Block SDK functionality into the application.
497-498: Added Block SDK mempool API route registration.This integrates the Block SDK's mempool API routes with the application's gRPC gateway router, allowing clients to query mempool data through the API.
528-534:✅ Verification successful
Added Block SDK mempool transaction service registration.
This code properly registers the Block SDK's mempool transaction service with the application's gRPC query router. The type assertion ensures that the app's mempool implements the required interface.
🏁 Script executed:
#!/bin/bash # Check where the mempool is configured to verify it implements block.Mempool rg -A 3 'SetMempool\(' --type goLength of output: 174
Mempool Transaction Service Registration Verified
The implementation correctly registers the Block SDK’s mempool transaction service. The type assertion confirms that the mempool implements the required interface before registration, and the configuration viaapp.SetMempool(mempool)is in place as expected. No further changes are necessary.scripts/protoc-swagger-gen.sh (2)
25-25: Extracting Block SDK VersionThe command to extract
BLOCK_SDK_VERSIONfromgo.modusinggrepandsedis consistent with how versions for other dependencies are extracted. Please verify that the module path ingo.modexactly matches the pattern ($BLOCK_SDK_URL/$BLOCK_SDK_V v) so that the correct version is captured.
52-52: Including Block SDK Proto FilesIncorporating the Block SDK proto directory into the
proto_dirsvariable ensures that its proto files are included in the swagger generation process. This integration is consistent with how other third-party proto directories are handled.client/docs/swagger-ui/swagger.yaml (1)
46966-47117: Enhanced Documentation for New Endpoints
The new endpoints for auction parameters (/block-sdk/auction/v1/params) and mempool distribution (/block-sdk/mempool/v1/distribution) are well articulated. The summaries, operation IDs, and detailed response schemas are clear and provide useful context to API consumers. Please verify the following:
- The descriptions (using the
>-multiline syntax) effectively convey the intended semantics for each property.- The default error response schema is consistent with your project’s error handling guidelines.
- The tags (
Queryfor the auction endpoint andServicefor the mempool endpoint) align with your API categorization standards.go.mod (7)
22-22: Dependency Update: cosmos-db v1.1.1The dependency for
github.com/cosmos/cosmos-dbhas been bumped to v1.1.1. Please verify the release notes for any potential breaking changes or subtle behavioral differences compared to v1.1.0.
24-24: Dependency Update: cosmos-sdk v0.50.12The
github.com/cosmos/cosmos-sdkversion has been updated to v0.50.12. This patch-level upgrade should ideally be backward-compatible; however, ensure that all modules interfacing with the SDK have been tested against this version.
27-27: Dependency Update: packet-forward-middleware v8.1.1The version of
github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v8has been updated to v8.1.1. Confirm that any new features or deprecations in this upgrade are compatible with our Block SDK integration.
36-36: Dependency Update: initia-labs/OPinit v0.7.2The
github.com/initia-labs/OPinitdependency now uses v0.7.2 instead of the previous v0.7.0. This minor bump appears to include refinements or fixes; please ensure that corresponding unit tests have passed.
43-43: Dependency Update: noble-assets/forwarding/v2 v2.0.1The update to
github.com/noble-assets/forwarding/v2v2.0.1 should deliver bug fixes or improvements over v2.0.0. Verify that any API usage or behavior changes are properly addressed in the consuming modules.
49-49: Dependency Update: spf13/cast v1.7.1Upgraded
github.com/spf13/castto version v1.7.1. As this is a patch update, it is expected to be backward compatible; still, please double-check that any utility functions used from this package are not affected by subtle changes.
103-103: Indirect Dependency Update: cosmos/ledger-cosmos-go v0.14.0The
github.com/cosmos/ledger-cosmos-godependency (marked as indirect) has been updated to v0.14.0. This update may include important security or stability improvements. It is advisable to verify its compatibility with any ledger integrations.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #97 +/- ##
==========================================
+ Coverage 64.89% 65.70% +0.81%
==========================================
Files 39 39
Lines 3176 3135 -41
==========================================
- Hits 2061 2060 -1
+ Misses 938 898 -40
Partials 177 177
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
client/docs/swagger-ui/swagger.yaml (1)
46766-46917: Review of New Endpoint Definitions for Auction and Mempool APIs
The newly added endpoints (/block-sdk/auction/v1/paramsand/block-sdk/mempool/v1/distribution) are well defined with clear response schemas and detailed property descriptions. The use of multi-line descriptions for complex fields (e.g.,max_bundle_size,escrow_account_address, etc.) enhances readability. One suggestion is to verify that the error response schema (used in thedefaultresponses) is consistent with other parts of your API documentation for uniform error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
client/docs/swagger-ui/swagger.yaml(3 hunks)go.mod(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run test and upload codecov
- GitHub Check: Minitiad
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/miniwasm/tokenfactory/v1/tx.pulsar.go (1)
6714-6727: Authority and params fields added to MsgUpdateParamsThe MsgUpdateParams message has been extended with two fields: 'authority' that controls the module and 'params' for defining the parameters to update. This adds proper governance capability to the tokenfactory module.
However, there's a minor documentation issue: the comment for the 'params' field refers to "x/staking parameters" instead of tokenfactory parameters. This should be corrected in the source proto file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/docker.yml(0 hunks)api/miniwasm/tokenfactory/v1/tx.pulsar.go(2 hunks)app/app.go(3 hunks)app/genesis.go(1 hunks)app/keepers/keepers.go(0 hunks)app/upgrade.go(2 hunks)x/bank/keeper/keeper.go(0 hunks)x/tokenfactory/types/msgs.go(0 hunks)
💤 Files with no reviewable changes (4)
- x/tokenfactory/types/msgs.go
- .github/workflows/docker.yml
- app/keepers/keepers.go
- x/bank/keeper/keeper.go
🚧 Files skipped from review as they are similar to previous changes (3)
- app/genesis.go
- app/upgrade.go
- app/app.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: golangci-lint
- GitHub Check: Run test and upload codecov
🔇 Additional comments (2)
api/miniwasm/tokenfactory/v1/tx.pulsar.go (2)
6909-6914: Updated amino name from "move/MsgUpdateParams" to "tokenfactory/MsgUpdateParams"The amino name has been correctly updated to align with the token factory context, which improves the naming consistency in the codebase.
6726-6726:❓ Verification inconclusive
Verify enforcement of the "All parameters must be supplied" requirement
The comment states that all parameters must be supplied, but it's not clear if this is enforced at the message handler level. Ensure validation checks are in place to verify all required parameters are provided when processing this message.
🏁 Script executed:
#!/bin/bash # Search for the MsgUpdateParams handler to verify validation logic rg -A 10 "UpdateParams.*Handler" --type goLength of output: 2712
Action: Validate parameter presence in UpdateParams
The auto-generated gRPC handler in
api/miniwasm/tokenfactory/v1/tx_grpc.pb.go(and similarly inx/tokenfactory/types/tx.pb.go) merely decodes the incomingMsgUpdateParamswithout performing any explicit checks on its fields before passing it to the service implementation. Please verify that the actualUpdateParamslogic (in the corresponding service implementation) enforces the "All parameters must be supplied" requirement—specifically, that theParamsfield is not nil. If this validation isn’t currently performed in the service logic, consider adding appropriate checks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/ibc-hooks/receive.go (1)
30-30: Improved error handling for memo parsing.This change improves the conditional logic to ensure that if an error occurs during memo parsing, the packet won't be processed through the default path even if
hookData.Messageis nil. This provides more robust error handling by ensuring errors aren't silently ignored.For even better code clarity, consider extracting this condition to a helper function with a descriptive name (e.g.,
shouldUseDefaultPacketHandler) to better communicate the intent:-if !isWasmRouted || (err == nil && hookData.Message == nil) { +if shouldUseDefaultPacketHandler(isWasmRouted, err, hookData) {Then define the function as:
func shouldUseDefaultPacketHandler(isWasmRouted bool, err error, hookData *ibchooks.IBCHookMessage) bool { return !isWasmRouted || (err == nil && hookData.Message == nil) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/ibc-hooks/receive.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run test and upload codecov
🔇 Additional comments (2)
app/ibc-hooks/receive.go (2)
94-94: Improved error handling for memo parsing.This change applies the same error handling improvement as in the
onRecvIcs20Packetfunction, ensuring consistent behavior across both ICS-20 and ICS-721 packet handlers. Good to see this consistency maintained.
30-34:❓ Verification inconclusive
Verify current error handling behavior across dependent systems.
The logic change ensures errors are properly caught and handled rather than proceeding with default packet handling. This is a good improvement for reliability, but it might change system behavior if any dependent systems were relying on the previous behavior where errors with nil messages would be handled by the default path.
Also applies to: 94-98
🏁 Script executed:
#!/bin/bash # Description: Look for any tests or code that might depend on the previous behavior # Check if there are any tests for these specific functions echo "Checking for tests that might need updating..." rg -t go "Test.*onRecvIcs20Packet|Test.*onRecvIcs721Packet" --no-filename # Look for any possible callers or references to these functions echo "Checking for callers or references..." rg -t go "onRecvIcs20Packet|onRecvIcs721Packet" --no-filename | grep -v "func.*onRecvIcs" | head -10Length of output: 500
Impact of Revised Error Handling on Dependent Systems
The changes in
app/ibc-hooks/receive.go(lines 30–34) now explicitly return an error acknowledgement when an error is encountered rather than falling back to the default packet handling viaim.App.OnRecvPacket. Our verification efforts show that:
- Callers such as
h.onRecvIcs20Packetandh.onRecvIcs721Packetare still active, so please double-check that these functions (and any dependent systems) do not rely on the previous behavior where errors with nil messages were silently handled.- Although no dedicated tests were flagged for these functions, the detected call sites suggest the need for careful integration testing and possible test updates.
- Similar logic changes at lines 94–98 should be verified for consistent behavior across the codebase.
Please ensure comprehensive testing and review of all affected components to confirm that the enhanced error handling does not introduce unintended side effects.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Dockerfile.arm64 (1)
12-12: Dynamic Versioning: Ensure theVERSIONvariable is providedThe updated build command now includes
VERSION=${VERSION}which enables dynamic versioning. To prevent potential build failures whenVERSIONisn’t set, consider declaring an argument (or providing a default value) using anARGinstruction in the Dockerfile. This also improves clarity by explicitly documenting the expected build-time variable.For example, you might add the following before the build command:
+ARG VERSIONshared.Dockerfile (1)
12-12: Dynamic Versioning Enhancement: Validate and Document theVERSIONUsageIn this Dockerfile, the build command now uses
VERSION=${VERSION}. To ensure that the build succeeds regardless of external environment settings, please confirm that theVERSIONvariable is passed during build time. If not already set by the CI/CD pipeline or build scripts, consider adding anARG VERSIONdeclaration (with an optional default value) near the top of the Dockerfile to safeguard against unset variables.A suggested addition is:
+ARG VERSIONDockerfile (1)
35-35: Ensure Proper Initialization ofVERSIONin the Build CommandThe modified build command now introduces a dependency on the
VERSIONenvironment variable:RUN VERSION=${VERSION} LEDGER_ENABLED=false BUILD_TAGS=muslc LDFLAGS="-linkmode=external -extldflags \"-L/code/mimalloc/build -lmimalloc -Wl,-z,muldefs -static\"" make buildTo avoid unexpected issues during the Docker build (for example, if
VERSIONisn’t provided), please consider adding anARG VERSIONdeclaration (with a default value if appropriate) near the beginning of the Dockerfile. This makes the requirements explicit and improves reliability.A recommended diff is:
+ARG VERSION
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
Dockerfile(1 hunks)Dockerfile.arm64(1 hunks)go.mod(3 hunks)shared.Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: golangci-lint
- GitHub Check: Run test and upload codecov
Description
This PR includes dependency upgrades and also includes audit fixes.
need to wait: initia-labs/initia#349
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...
Summary by CodeRabbit
New Features
HookMaxGasparameter to configuration settings for improved gas management.Refactor
Documentation
Chores