feat(evm-precompile): Emit EVM events created to reflect the ABCI events that occur outside the EVM to make sure that block explorers and indexers can find indexed ABCI event information. #2125
Conversation
…ts that occur outside the EVM to make sure that block explorers and indexers can find indexed ABCI event information.
WalkthroughThe pull request includes several changes across multiple files. Key modifications involve updating the Node.js version in Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
Unique-Divine
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
x/evm/keeper/keeper_test.go (2)
22-23: Use specific log levelInfofor clarityIn the
SetupTestmethod, it's better to specify the log level explicitly for clarity and consistency. Usinglog.Info().Msgfinstead oflog.Log().Msgfmakes it clear that the message is informational.Apply the following diff to update the log level:
func (s *Suite) SetupTest() { - log.Log().Msgf("SetupTest %v", s.T().Name()) + log.Info().Msgf("SetupTest %v", s.T().Name()) }
21-23: Avoid cluttering test output with logsWhile logging can be helpful during development, consider whether logging the setup of each test is necessary. Excessive logging can clutter test output, making it harder to identify failures. If the logs are for debugging purposes, you might want to control them with a verbosity flag or remove them before finalizing the tests.
x/evm/embeds/contracts/NibiruEvmUtils.sol (1)
12-23: Consider enhancing type safety for event attributesThe event design with
bytes attrsprovides flexibility but might make it harder for consumers to parse the data correctly. Consider:
- Adding helper functions for encoding/decoding common attribute types
- Documenting the expected attribute format for each
eventTypeWould you like me to propose a set of helper functions for common attribute types?
contrib/bashlib.sh (1)
32-51: LGTM: Consistent logging improvementsThe addition of
-eflag to enable ANSI color codes is applied consistently across all logging functions. Consider adding a test to verify color output functionality.Add this test function to verify color output:
test_color_output() { # Capture output with color codes local output=$(log_debug "test message") # Verify color codes are present [[ "$output" == *$'\033'* ]] || { echo "Color codes not working"; exit 1; } }x/evm/precompile/nibiru_evm_utils_test.go (1)
60-118: Consider adding more test cases for EmitEventAbciEventWhile the current test verifies basic event emission functionality, consider adding test cases for:
- Events with empty attributes
- Events with malformed data
- Maximum event size limits
- Error conditions
Here's a suggested test case structure:
func (s *UtilsSuite) TestEmitEventAbciEvent() { + testCases := []struct { + name string + setupEvents func(ctx sdk.Context) []abci.Event + wantLogs []*gethcore.Log + wantErr bool + }{ + { + name: "empty event attributes", + setupEvents: func(ctx sdk.Context) []abci.Event { + return []abci.Event{{ + Type: "test_event", + Attributes: []abci.EventAttribute{}, + }} + }, + wantLogs: []*gethcore.Log{ + { + // Expected log structure for empty attributes + }, + }, + }, + // Add more test cases here + }x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json (1)
6-24: Consider documenting event parametersAdd NatSpec documentation to explain:
- Purpose and structure of eventType
- Expected format of attrs
- Example usage
{ "anonymous": false, + "documentation": { + "details": "Emitted when an ABCI event occurs. The eventType parameter indicates the ABCI event type, and attrs contains the JSON-encoded attributes.", + "params": { + "eventType": "The type of ABCI event (e.g., 'coin_received', 'message')", + "attrs": "JSON-encoded event attributes" + } + }, "inputs": [ { "indexed": true,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.nvmrc(1 hunks)CHANGELOG.md(1 hunks)contrib/bashlib.sh(1 hunks)justfile(1 hunks)x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json(1 hunks)x/evm/embeds/artifacts/contracts/NibiruEvmUtils.sol/INibiruEvm.json(1 hunks)x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json(1 hunks)x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json(1 hunks)x/evm/embeds/artifacts/contracts/TestNativeSendThenPrecompileSend.sol/TestNativeSendThenPrecompileSend.json(1 hunks)x/evm/embeds/artifacts/contracts/TestPrecompileSelfCallRevert.sol/TestPrecompileSelfCallRevert.json(1 hunks)x/evm/embeds/artifacts/contracts/Wasm.sol/IWasm.json(4 hunks)x/evm/embeds/contracts/IFunToken.sol(1 hunks)x/evm/embeds/contracts/NibiruEvmUtils.sol(1 hunks)x/evm/embeds/contracts/Wasm.sol(1 hunks)x/evm/keeper/keeper_test.go(2 hunks)x/evm/keeper/msg_server.go(0 hunks)x/evm/precompile/funtoken.go(2 hunks)x/evm/precompile/funtoken_test.go(3 hunks)x/evm/precompile/nibiru_evm_utils.go(1 hunks)x/evm/precompile/nibiru_evm_utils_test.go(1 hunks)x/evm/precompile/wasm.go(2 hunks)
💤 Files with no reviewable changes (1)
- x/evm/keeper/msg_server.go
✅ Files skipped from review due to trivial changes (6)
- .nvmrc
- x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json
- x/evm/embeds/artifacts/contracts/TestPrecompileSelfCallRevert.sol/TestPrecompileSelfCallRevert.json
- x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json
- x/evm/embeds/artifacts/contracts/NibiruEvmUtils.sol/INibiruEvm.json
- x/evm/embeds/artifacts/contracts/TestNativeSendThenPrecompileSend.sol/TestNativeSendThenPrecompileSend.json
🔇 Additional comments (13)
x/evm/embeds/contracts/Wasm.sol (1)
8-68: Ensure correct import path and interface integration
The import statement import "./NibiruEvmUtils.sol"; brings in INibiruEvm, which IWasm now extends. Verify that the imported file exists at the specified path and that no circular dependencies are introduced. The inheritance and updated method signatures appear consistent, and integration with INibiruEvm enhances functionality.
Run the following script to ensure the import path is correct and the code compiles without errors:
✅ Verification successful
Import path and interface integration are correctly implemented
The verification confirms:
NibiruEvmUtils.solexists at the correct relative pathINibiruEvminterface is properly defined inNibiruEvmUtils.solwith the requiredBankCoinstructIWasmcorrectly extendsINibiruEvm- No circular dependencies were found as there are no imports of
Wasm.solin other files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `Wasm.sol` compiles successfully with the new import.
# Assuming you have `solc` installed and the Solidity files in `./contracts` directory.
solc --overwrite --abi --bin -o ./build ./contracts/Wasm.sol
Length of output: 157
Script:
#!/bin/bash
# Let's verify the import path and interface integration through file existence and content analysis
# Check if the imported file exists
fd "NibiruEvmUtils.sol"
# Check the content of both files to verify the interface inheritance
echo "=== Content of NibiruEvmUtils.sol ==="
rg -A 5 "interface INibiruEvm"
echo "=== Content showing IWasm inheritance ==="
rg -A 5 "interface IWasm"
# Check for potential circular dependencies
echo "=== Checking for potential circular imports ==="
rg "import.*Wasm.sol"
Length of output: 1398
x/evm/precompile/funtoken.go (1)
Line range hint 63-95: ABCI event emission logic correctly implemented
The added logic in the Run method for emitting ABCI events to the EVM state database is appropriate. Capturing the index of existing events before execution and emitting new events afterward ensures only relevant events are processed. The conditional check !readonly correctly prevents event emission during read-only calls.
x/evm/embeds/contracts/NibiruEvmUtils.sol (1)
16-20: Verify event type format compatibility with block explorers
The documentation mentions two event type formats:
- Snake case (e.g., "pending_ethereum_tx")
- Protobuf message names (e.g., "eth.evm.v1.EventEthereumTx")
✅ Verification successful
Based on the search results, I can see that both event type formats mentioned in the documentation are actively used in the codebase:
- Snake case format is used in various places, particularly for module-specific events
- Protobuf message format (e.g. "eth.evm.v1.EventEthereumTx") is used extensively in the EVM module
Let me verify one more aspect to ensure complete coverage:
Let me check one final aspect to see the actual event emission implementations:
Event type formats are correctly documented and consistently used
The documentation accurately reflects the two event type formats used in the codebase:
- Snake case format (e.g., "pending_ethereum_tx") is used for CometBFT indexer events, as seen in
evm.PendingEthereumTxEvent - Protobuf message format (e.g., "eth.evm.v1.EventEthereumTx") is used for typed events, as evidenced by numerous protobuf event definitions in the codebase
The implementation consistently follows these conventions across different modules, ensuring compatibility with block explorers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent event type usage across the codebase
echo "Searching for event type declarations..."
rg -A 1 'eventType.*=.*"[^"]*"'
# Check for event type string literals
echo "Checking event type format consistency..."
rg '"[a-z_]+\.[a-z_]+\.v[0-9]+\.[A-Za-z]+"'
rg '"[a-z_]+"'
Length of output: 65864
Script:
# Check for event emission in Go code
rg 'EventTypeKey.*=.*"[^"]*"'
rg 'ctx\.EventManager\(\)\.EmitEvent\('
Length of output: 256
Script:
# Check the event emission implementations
rg -A 5 'ctx\.EventManager\(\)\.EmitEvent\('
Length of output: 1455
x/evm/embeds/contracts/IFunToken.sol (1)
7-12: LGTM: Clean integration of ABCI event capabilities
The inheritance from INibiruEvm is a clean way to add ABCI event capabilities while maintaining the existing token functionality. The import statement is correctly placed, and the interface hierarchy is well-structured.
x/evm/precompile/nibiru_evm_utils_test.go (1)
26-58: LGTM: TestAttrsToJSON has good test coverage
The test cases effectively verify the JSON conversion logic with edge cases:
- Handling of repeated keys
- Multiple unique attributes
- JSON structure validation
x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json (1)
6-24: Verify event parameter indexing strategy
The current indexing strategy allows searching by eventType but not attrs. Consider if this meets the requirements for block explorers and indexers.
✅ Verification successful
Event indexing strategy is appropriate
The current indexing strategy (indexed eventType, non-indexed attrs) aligns with both the codebase patterns and technical constraints:
- The same event structure is used in NibiruEvmUtils.sol:
event AbciEvent(string indexed eventType, bytes attrs) - The indexing strategy is appropriate because:
eventTypebeing indexed enables efficient filtering/searching of specific event typesbytestype parameters are typically not indexed due to their variable size nature and gas costs- Other events in the codebase (TestEvent, ValueUpdateAttempted) follow similar patterns of indexing fixed-size parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other contracts in the codebase follow similar indexing patterns
ast-grep --pattern 'event $name($$$) {
$$$
}'
# Check if there are any existing events with bytes as indexed parameters
ast-grep --pattern 'event $name($$$) {
indexed bytes $varname
$$$
}'
Length of output: 124
Script:
#!/bin/bash
# Let's try a different approach to find events in Solidity files
# First, find all Solidity files
fd -e sol -x cat {} | grep -A 5 "event.*{.*"
# Also search for any files containing "event" to ensure we don't miss anything
rg "event.*{" -A 5
Length of output: 1108
Script:
#!/bin/bash
# Let's try a more specific search in the actual source files
# First, let's find the actual Solidity source files
fd -e sol -x realpath {} | grep -v "artifacts"
# Then search for events with proper regex
rg "event\s+\w+\s*\([^)]*\)" -A 5
# Also check the specific contract file
cat x/evm/embeds/contracts/IFunToken.sol
Length of output: 4291
x/evm/embeds/artifacts/contracts/Wasm.sol/IWasm.json (2)
6-24: LGTM: Well-structured event declaration for ABCI events
The new AbciEvent is well-designed with:
- Indexed
eventTypefield for efficient filtering - Non-indexed
attrsfield for flexible attribute storage
50-50: LGTM: Consistent type refactoring across functions
The change from IWasm.BankCoin[] to INibiruEvm.BankCoin[] promotes code reuse by utilizing a common type.
Also applies to: 93-93, 149-149
x/evm/precompile/wasm.go (2)
49-50: LGTM: Efficient event tracking implementation
Capturing the initial event count before operations allows precise tracking of new events.
79-86: LGTM: Well-placed conditional event emission
The implementation:
- Correctly skips event emission for read-only transactions
- Captures all events by emitting after operations complete
- Uses proper event range based on tracked start index
x/evm/precompile/funtoken_test.go (2)
25-25: LGTM: Enhanced test coverage
Added UtilsSuite to the test suite improves test coverage.
228-231: LGTM: Proper fee collector funding
Using sdkmath.NewInt ensures precise handling of large numbers in the SDK context.
CHANGELOG.md (1)
54-54: LGTM! Well-formatted changelog entry.
The changelog entry for PR #2125 is properly formatted, placed in the correct section, and clearly describes the feature's purpose of emitting EVM events to reflect ABCI events for block explorer visibility.
| emittingAddr gethcommon.Address, | ||
| ) { | ||
| blockNumber := uint64(ctx.BlockHeight()) | ||
| event := embeds.SmartContract_Wasm.ABI.Events[EvmEventAbciEvent] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing event definition
In the EmitEventAbciEvents function, there is no check to ensure that the AbciEvent is present in the ABI. If the event definition is missing, it could lead to a runtime panic. Consider adding error handling to verify that the event is found before proceeding.
Apply this diff to add the check:
event := embeds.SmartContract_Wasm.ABI.Events[EvmEventAbciEvent]
+if event == nil {
+ // Handle the error appropriately, e.g., log or return an error
+ return
+}Committable suggestion skipped: line range outside the PR's diff.
| for i, attr := range attrs { | ||
| // Keys must be unique to guarantee valid JSON object | ||
| if keysSeen.Has(attr.Key) { | ||
| continue | ||
| } | ||
| keysSeen.Add(attr.Key) | ||
|
|
||
| if i > 0 { | ||
| buf.WriteByte(',') | ||
| } | ||
|
|
||
| // Quote key and value | ||
| _, _ = fmt.Fprintf(&buf, `"%s":"%s"`, attr.Key, attr.Value) | ||
| } | ||
| buf.WriteByte('}') | ||
|
|
||
| return buf.Bytes() | ||
| } |
There was a problem hiding this comment.
Fix incorrect comma placement when skipping duplicate keys
In the AttrsToJSON function, the current logic for inserting commas between JSON key-value pairs uses the loop index i, which counts all attributes, including those skipped due to duplicate keys. This can lead to improperly formatted JSON with extra commas when duplicate keys are present.
To ensure valid JSON, track the number of entries actually added to the buffer instead of using the loop index.
Apply the following diff to correct the issue:
var buf bytes.Buffer
buf.WriteByte('{')
+count := 0
for _, attr := range attrs {
// Keys must be unique to guarantee valid JSON object
if keysSeen.Has(attr.Key) {
continue
}
keysSeen.Add(attr.Key)
- if i > 0 {
+ if count > 0 {
buf.WriteByte(',')
}
+ count++
// Quote key and value
_, _ = fmt.Fprintf(&buf, `"%s":"%s"`, attr.Key, attr.Value)
}
buf.WriteByte('}')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i, attr := range attrs { | |
| // Keys must be unique to guarantee valid JSON object | |
| if keysSeen.Has(attr.Key) { | |
| continue | |
| } | |
| keysSeen.Add(attr.Key) | |
| if i > 0 { | |
| buf.WriteByte(',') | |
| } | |
| // Quote key and value | |
| _, _ = fmt.Fprintf(&buf, `"%s":"%s"`, attr.Key, attr.Value) | |
| } | |
| buf.WriteByte('}') | |
| return buf.Bytes() | |
| } | |
| var buf bytes.Buffer | |
| buf.WriteByte('{') | |
| count := 0 | |
| for _, attr := range attrs { | |
| // Keys must be unique to guarantee valid JSON object | |
| if keysSeen.Has(attr.Key) { | |
| continue | |
| } | |
| keysSeen.Add(attr.Key) | |
| if count > 0 { | |
| buf.WriteByte(',') | |
| } | |
| count++ | |
| // Quote key and value | |
| _, _ = fmt.Fprintf(&buf, `"%s":"%s"`, attr.Key, attr.Value) | |
| } | |
| buf.WriteByte('}') | |
| return buf.Bytes() | |
| } |
| # Generate Solidity artifacts for x/evm/embeds | ||
| gen-embeds: | ||
| #!/usr/bin/env bash | ||
| source contrib/bashlib.sh | ||
|
|
||
| embeds_dir="x/evm/embeds" | ||
| log_info "Begin to compile Solidity in $embeds_dir" | ||
| which_ok npm | ||
| log_info "Using system node version: $(npm exec -- node -v)" | ||
|
|
||
| cd "$embeds_dir" || (log_error "path $embeds_dir not found" && exit) | ||
| npx hardhat compile | ||
| log_success "Compiled Solidity in $embeds_dir" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in gen-embeds recipe
The recipe should handle common failure scenarios:
- Node.js version mismatch
- Missing dependencies
- Compilation errors
gen-embeds:
#!/usr/bin/env bash
source contrib/bashlib.sh
embeds_dir="x/evm/embeds"
log_info "Begin to compile Solidity in $embeds_dir"
- which_ok npm
+ if ! which_ok npm; then
+ log_error "npm not found. Please install Node.js"
+ exit 1
+ fi
+
+ if [ ! -f "package.json" ]; then
+ log_error "package.json not found in $embeds_dir"
+ exit 1
+ fi
+
log_info "Using system node version: $(npm exec -- node -v)"
cd "$embeds_dir" || (log_error "path $embeds_dir not found" && exit)
+ npm install || (log_error "Failed to install dependencies" && exit 1)
npx hardhat compile
log_success "Compiled Solidity in $embeds_dir"Committable suggestion skipped: line range outside the PR's diff.
Purpose / Abstract
feat(evm-precompile): Emit EVM events created to reflect the ABCI events that occur outside the EVM to make sure that block explorers and indexers can find indexed ABCI event information.
Summary by CodeRabbit
New Features
AbciEventin multiple contracts for enhanced event logging.Bug Fixes
Documentation
CHANGELOG.mdwith new entries detailing recent features, bug fixes, and improvements.Tests
Chores
.nvmrcto specify the latest LTS Node.js version.