feat: update ante handler to allow authz message #116
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
Warning Rate limit exceeded@beer-1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (2)
integration-tests/go.mod (1)
Line range hint
290-294: Verify grpc version constraintThe grpc version constraint to v1.65.0 is added to address issues with cosmos/relayer. This is a reasonable workaround, but we should track when this can be removed.
Consider adding a TODO comment with a link to track when this constraint can be removed.
go.mod (1)
Line range hint
292-295: Consider documenting the grpc version constraintThe comment explains the reason for pinning the grpc version, but it would be helpful to add a link to the issue or PR tracking this limitation.
// cosmos/relayer seems having problem with the latest version of grpc; return nil in the below line // - https://github.com/cosmos/relayer/blob/4e4e9530800d28fb2c984f1cfc7b03f05eec618c/relayer/chains/cosmos/grpc_query.go#L30 + // TODO: Remove this constraint once https://github.com/cosmos/relayer/issues/<number> is resolved google.golang.org/grpc => google.golang.org/grpc v1.65.0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumintegration-tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(1 hunks)integration-tests/go.mod(1 hunks)
🔇 Additional comments (6)
integration-tests/go.mod (3)
Line range hint 266-271: Security fixes look good
The replace directives appropriately address known security vulnerabilities:
- Replacing deprecated
jwt-gowith maintainedgolang-jwt/jwt/v4 - Fixing upstream vulnerability in
gin-gonic/gin
Line range hint 284-288: Verify custom fork compatibility
The replace directives for custom forks need careful consideration:
cometbft→initia-labs/cometbftibc-go/v8→initia-labs/ibc-go/v8go-ethereum→initia-labs/evm
#!/bin/bash
# Description: Verify the compatibility and stability of custom forks
# Check commit history of custom forks for recent updates
for repo in cometbft ibc-go evm; do
echo "=== Checking $repo ==="
gh api repos/initia-labs/$repo/commits --jq '.[0:3].[].commit.message'
done
# Look for any reported issues with these forks
for repo in cometbft ibc-go evm; do
echo "=== Issues for $repo ==="
gh api repos/initia-labs/$repo/issues --jq '.[] | select(.state=="open") | {title,created_at}'
done149-149: Verify compatibility with OPinit v0.6.1
The dependency update to github.com/initia-labs/OPinit v0.6.1 aligns with the PR objectives mentioning related changes in OPinit PR #126.
go.mod (3)
Line range hint 261-278: Security and bug fixes in replace directives look appropriate
The replace directives address known security issues and bugs:
- Replacing deprecated
jwt-gowith maintainedgolang-jwt - Fixing GHSA-h395-qcrw-5vmq vulnerability in
gin-gonic/gin - Using cosmos fork of keyring for better security
- Downgrading
goleveldbto avoid simulation failures
39-39: Verify compatibility with Go 1.23.0
The dependency github.com/initia-labs/OPinit has been updated to v0.6.1, which is required for the ante handler changes. However, we should ensure all dependencies are compatible with Go 1.23.0.
#!/bin/bash
# Description: Check for any compatibility issues with Go 1.23.0
# Search for any go.mod files in the codebase
echo "Checking for additional go.mod files..."
fd -t f "go.mod$" --exec sh -c '
echo "=== {} ==="
cat "{}" | grep -E "^go |toolchain" || true
'
# Check if any dependencies explicitly require an older Go version
echo "Checking dependency constraints..."
go mod graph | grep -v "github.com/initia-labs/minievm" | sort -uLine range hint 280-295: Verify custom replacements for Initia components
The custom replacements point to specific commits of Initia-maintained forks:
cometbft→initia-labs/cometbft@a371e2dcibc-go/v8→initia-labs/ibc-go/v8@19c0b4adgo-ethereum→initia-labs/evm@3d312736
These changes appear to be related to the ante handler updates.
✅ Verification successful
Let me try a different approach to verify these replacements.
Based on the extensive search results, I can now verify the custom replacements. Let me analyze the usage of these replaced modules:
-
The
cometbftreplacement is used extensively throughout the codebase for core blockchain functionality, particularly in app initialization, ABCI interfaces, and testing. -
The
ibc-go/v8replacement is used in IBC-related functionality, especially in the app's IBC hooks and keeper initialization. -
The
go-ethereumreplacement is heavily used in the EVM module for implementing Ethereum compatibility, including state management, JSON-RPC endpoints, and ERC20/ERC721 contracts.
The replacements are integral to the codebase's functionality and are used consistently. The commit hashes point to specific versions of Initia's forks that contain customizations needed for the application.
Custom replacements are correctly configured and actively used
The replacements are valid because:
- Each replaced module is extensively used throughout the codebase
- The replacements point to Initia's maintained forks with specific commits
- The usage patterns show these are core dependencies integrated into the application's functionality
- The replacements follow a consistent pattern of using Initia's customized versions for core blockchain components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the referenced commits exist and contain relevant changes
# Check if the commits exist in the repositories
echo "Verifying commit existence..."
for repo in "cometbft" "ibc-go" "evm"; do
echo "=== $repo ==="
gh api repos/initia-labs/$repo/commits/$(rg -o '[0-9a-f]{40}' | grep -w "$repo" -A 1 | tail -n 1) --jq '.commit.message'
done
# Search for ante handler related changes
echo "Checking for ante handler changes..."
rg -l "ante.*handler"
Length of output: 1868
Script:
#!/bin/bash
# Check the actual commit hashes from go.mod
echo "=== Commit hashes from go.mod ==="
cat go.mod | grep -A 1 "initia-labs"
# Check for recent changes in ante handler files
echo -e "\n=== Recent ante handler changes ==="
for file in $(rg -l "ante.*handler" | grep -v "swagger" | grep -v "docs"); do
echo -e "\n--- $file ---"
git log -p -1 $file
done
# Check imports of replaced modules
echo -e "\n=== Usage of replaced modules ==="
rg -l "github.com/(cometbft/cometbft|cosmos/ibc-go/v8|ethereum/go-ethereum)" --type go
Length of output: 60494
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/upgrade.go (2)
Line range hint
49-61: Consider adding logs for skipped deployments.The error handling correctly ignores contract address collisions while propagating other errors. However, it would be helpful to log when deployments are skipped due to existing contracts.
if err := app.EVMKeeper.DeployERC20Factory(ctx); err != nil && // ignore contract address collision error (contract already deployed) - !strings.Contains(err.Error(), vm.ErrContractAddressCollision.Error()) { + !strings.Contains(err.Error(), vm.ErrContractAddressCollision.Error()) { return nil, err +} else if err != nil { + app.Logger(ctx).Info("Skipping ERC20Factory deployment - contract already exists") }
Line range hint
63-108: Critical: Add safety measures for contract code updates.The contract update logic has several areas that could be improved for safety and monitoring:
- Add validation for the extracted runtime code before deployment
- Consider adding a backup mechanism
- Add progress tracking for large-scale updates
- Add validation that the new code maintains the same interface
Consider these improvements:
- Add code validation:
code = code[initCodePos+3:] +// Validate minimum code size +if len(code) < 100 { // adjust minimum size based on your contract + return nil, fmt.Errorf("extracted runtime code too small: %d bytes", len(code)) +}
- Add progress tracking:
+var processedContracts int64 err = app.EVMKeeper.ERC20s.Walk(ctx, nil, func(contractAddr []byte) (bool, error) { + processedContracts++ + if processedContracts%100 == 0 { + app.Logger(ctx).Info("Processing ERC20 contracts", "processed", processedContracts) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/keepers/keepers.go(1 hunks)app/upgrade.go(1 hunks)
🔇 Additional comments (4)
app/upgrade.go (3)
Line range hint 110-114: LGTM! Correct implementation of uint64 serialization.
The helper function correctly implements uint64 to bytes conversion using BigEndian encoding.
Line range hint 35-47: Verify the default HookMaxGas value.
The code correctly updates OPChild parameters with error handling, but please ensure that opchildtypes.DefaultHookMaxGas is an appropriate value for production use.
#!/bin/bash
# Check the DefaultHookMaxGas value
ast-grep --pattern 'const DefaultHookMaxGas = $_'25-25: LGTM! Verify version consistency.
The version bump from "0.6.5" to "0.6.6" is appropriate for these changes.
✅ Verification successful
Version "0.6.6" appears to be consistently defined
The version string "0.6.6" is only defined in app/upgrade.go and there are no conflicting version numbers found across the codebase. The version bump appears to be cleanly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify version consistency across the codebase
rg "0\.6\.[56]" --type yaml --type go
Length of output: 51
Script:
#!/bin/bash
# Search for version strings with more flexible patterns
rg -i "0\.6\.[0-9]|version.*0\.6|v0\.6" --type yaml --type go
# Also check for specific version numbers in version-related files
fd -e yaml -e go version
Length of output: 133
app/keepers/keepers.go (1)
287-287: LGTM! Necessary setup for authorization message handling.
The addition of SetBankKeeper to the AuthzKeeper is correct and necessary for supporting bank-related authorization messages and fee granter verification.
Let's verify the authorization message handling setup:
✅ Verification successful
Based on the code analysis from the shell script results, I can now provide a final response:
Setting BankKeeper in AuthzKeeper is correct and necessary
The code shows that the AuthzKeeper requires the BankKeeper for proper authorization handling, as evidenced by:
- The keeper initialization in
app/keepers/keepers.gofollows the correct dependency order:BankKeeperis initialized firstAuthzKeeperis initialized laterBankKeeperis explicitly set viaSetBankKeepermethod
This setup is necessary because:
- It enables the authorization module to verify bank-related permissions
- It's part of the standard Cosmos SDK authorization pattern
- The dependency is properly managed through the builder pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify authorization message handling configuration
# Test 1: Check for authz message handlers that require bank keeper
ast-grep --pattern 'func (k Keeper) $$$Bank$$$($$$) $$$'
# Test 2: Check for fee granter verification logic
rg -A 5 'VerifyFeeGranter|ValidateFeeGranter'
Length of output: 106
Script:
#!/bin/bash
# Let's try a broader search to understand the authorization and bank keeper integration
# Check for authz keeper implementation and its bank keeper dependency
rg -l "BankKeeper" --type go
# Look for authorization message handling patterns
rg -A 5 "type AuthzKeeper|type BankKeeper" --type go
# Check for fee granter related code
rg -l "FeeGranter" --type go
# Look for ante handler implementations related to authz
rg -A 5 "AnteHandler.*authz|ProcessFeeGranter" --type go
Length of output: 1246
Script:
#!/bin/bash
# Let's check the specific implementation in app/ante files since they handle fee and authorization logic
# Check ante.go and fee.go for authorization and fee granter logic
cat app/ante/ante.go
cat app/ante/fee.go
# Check the authz keeper initialization and its dependencies
cat app/keepers/keepers.go
# Look for any authz-related interfaces or types
rg -A 10 "interface.*Authz|type.*Authz" --type go
Length of output: 28759
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #116 +/- ##
==========================================
- Coverage 28.16% 28.15% -0.02%
==========================================
Files 126 126
Lines 14285 14301 +16
==========================================
+ Hits 4023 4026 +3
- Misses 9686 9699 +13
Partials 576 576
|
Description
Bump OPinit to allow authz message to system lane and check fee granter when we check fee whitelist in free lane.
initia-labs/OPinit#126
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
github.com/initia-labs/OPinitto enhance functionality.Bug Fixes
github.com/gin-gonic/ginand replacing deprecated packages.Chores