Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

patch address & topics parsing in parseLog #294

Merged
merged 1 commit into from
Dec 20, 2024
Merged

Conversation

RnkSngh
Copy link
Collaborator

@RnkSngh RnkSngh commented Dec 20, 2024

Summary by CodeRabbit

  • New Features
    • Introduced a new function bytesToAddr for converting byte arrays to Ethereum addresses.
    • Added a new error type invalidAddressBytes for improved error handling.
  • Bug Fixes
    • Improved memory management in the parseLog function.
  • Updates
    • Updated bytecode for multiple contracts, indicating changes in their implementations.
    • Incremented version number in package.json from "4.0.18" to "4.0.19".

Copy link

coderabbitai bot commented Dec 20, 2024

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 eslint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

src/evm/contracts/Ibc.ts

(node:28589) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///eslint.config.js?mtime=1734656932436 is not specified and it doesn't parse as CommonJS.
Reparsing as ES module because module syntax was detected. This incurs a performance overhead.
To eliminate this warning, add "type": "module" to /package.json.
(Use node --trace-warnings ... to show where the warning was created)

Oops! Something went wrong! :(

ESLint: 8.57.1

TypeError: Key "rules": Key "@typescript-eslint/quotes": Could not find "quotes" in plugin "@typescript-eslint". Did you mean "@/quotes"?
at throwRuleNotFoundError (/node_modules/eslint/lib/config/rule-validator.js:66:11)
at RuleValidator.validate (/node_modules/eslint/lib/config/rule-validator.js:128:17)
at [finalizeConfig] (/node_modules/eslint/lib/config/flat-config-array.js:337:23)
at FlatConfigArray.getConfig (/node_modules/@humanwhocodes/config-array/api.js:1036:55)
at /node_modules/eslint/lib/eslint/flat-eslint.js:812:40
at Array.map ()
at FlatESLint.lintFiles (/node_modules/eslint/lib/eslint/flat-eslint.js:810:23)
at async Object.execute (/node_modules/eslint/lib/cli.js:421:23)
at async main (/node_modules/eslint/bin/eslint.js:152:22)

Walkthrough

This pull request introduces a new error type invalidAddressBytes and a bytesToAddr function in the Ibc library, which converts byte arrays to Ethereum addresses using inline assembly. The parseLog function has been updated to utilize this new method, enhancing address conversion and memory management. Additionally, the package version has been incremented from 4.0.18 to 4.0.19, and several contract factory files have been updated with new bytecode.

Changes

File Change Summary
contracts/libs/Ibc.sol Added bytesToAddr function and invalidAddressBytes error
package.json Version updated from 4.0.18 to 4.0.19
src/evm/contracts/Ibc.ts Added bytesToAddr method to IbcInterface
src/evm/contracts/factories/CrossL2Prover__factory.ts Added invalidAddressBytes error
src/evm/contracts/factories/Dispatcher__factory.ts Updated bytecode and refined constructor logic
src/evm/contracts/factories/Earth__factory.ts Updated bytecode for Earth contract
src/evm/contracts/factories/FeeVault__factory.ts Updated bytecode for FeeVault contract
src/evm/contracts/factories/IbcUtils__factory.ts Updated bytecode for IbcUtils contract
src/evm/contracts/factories/Ibc__factory.ts Added bytesToAddr method and invalidAddressBytes error
src/evm/contracts/factories/Mars.sol/Mars__factory.ts Updated bytecode for Mars contract
src/evm/contracts/factories/Mars.sol/PanickingMars__factory.ts Updated bytecode for PanickingMars contract
src/evm/contracts/factories/Mars.sol/RevertingBytesMars__factory.ts Updated bytecode for RevertingBytesMars contract
src/evm/contracts/factories/Mars.sol/RevertingEmptyMars__factory.ts Updated bytecode for RevertingEmptyMars contract
src/evm/contracts/factories/Mars.sol/RevertingStringCloseChannelMars__factory.ts Updated bytecode for RevertingStringCloseChannelMars contract
src/evm/contracts/factories/Mars.sol/RevertingStringMars__factory.ts Updated bytecode for RevertingStringMars contract
src/evm/contracts/factories/Moon__factory.ts Updated bytecode for Moon contract
src/evm/contracts/factories/UniversalChannelHandler__factory.ts Updated bytecode and constructor logic
bindings/go/crossl2prover/CrossL2Prover.go Added invalidAddressBytes error to ABI

Possibly related PRs

  • add fee vault threshold to prevent indexer spam #189: The changes in the main PR introduce a new error type invalidAddressBytes in the Ibc library, which is relevant to the error handling improvements in the CrossL2Prover factory, where a similar error type is added.
  • Raunak/informal q3 fixes #192: The main PR's introduction of the invalidAddressBytes error type aligns with the changes in the CrossL2Prover.go file, which also updates the ABI to include this error type.
  • Raunak/sequencer client l1 origin check #227: The changes in the main PR regarding error handling and address validation are related to the updates in the SequencerSoloClient contract, which also enhances error handling capabilities.
  • rename uch openChannel -> triggerChannelInit #233: The renaming of the openChannel function to triggerChannelInit in the Universal Channel Handler is relevant as it reflects a shift in functionality that could relate to the changes in the Ibc library's error handling.
  • fix go bindings #264: The modifications in the main PR regarding the invalidAddressBytes error type are relevant to the changes in the CrossL2Prover and ICrossL2Prover files, which also update their error handling mechanisms.
  • fix typo in chainId -> chainID in crossl2 prover #293: The typo fix in chainId to chainID in the CrossL2Prover is relevant as it pertains to the consistency of naming conventions, which is a part of the overall improvements in the main PR's error handling and validation logic.

Suggested Labels

enhancement, bug

Suggested Reviewers

  • dshiell

Poem

🐰 Bytes to Address, a Rabbit's Delight
Inline assembly, making conversion bright
From raw bytes to Ethereum's domain
Our code now dances with elegant refrain
A small change, but oh so neat! 🌟

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request. We would love to hear your feedback on Discord.


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (2)
src/evm/contracts/factories/Ibc__factory.ts (1)

121-139: Validate newly introduced ABI entry for "bytesToAddr".
These lines add the "bytesToAddr" function definition to the contract’s ABI. Verify that the function name, parameter, and return type match the actual contract implementation.

src/evm/contracts/factories/UniversalChannelHandler__factory.ts (1)

Line range hint 1012-1022: Ensure robust library address linking

The linkBytecode implementation uses string replacement for library addresses. While functional, consider adding validation for the library address format and existence.

Consider adding these improvements:

  1. Validate library address format
  2. Check for address existence before replacement
  3. Add error handling for missing library addresses
static linkBytecode(
  linkLibraryAddresses: UniversalChannelHandlerLibraryAddresses
): string {
  let linkedBytecode = _bytecode;

+ // Validate library address format
+ const libraryAddress = linkLibraryAddresses["contracts/libs/IbcUtils.sol:IbcUtils"];
+ if (!libraryAddress || !/^0x[0-9a-fA-F]{40}$/.test(libraryAddress)) {
+   throw new Error("Invalid library address format");
+ }

  linkedBytecode = linkedBytecode.replace(
    new RegExp("__\\$f61eb90c6f674e787d51c07f105fa231e2\\$__", "g"),
    libraryAddress.replace(/^0x/, "").toLowerCase()
  );

  return linkedBytecode;
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between edc2503 and b9d478c.

📒 Files selected for processing (17)
  • contracts/libs/Ibc.sol (2 hunks)
  • package.json (1 hunks)
  • src/evm/contracts/Ibc.ts (5 hunks)
  • src/evm/contracts/factories/CrossL2Prover__factory.ts (1 hunks)
  • src/evm/contracts/factories/Dispatcher__factory.ts (1 hunks)
  • src/evm/contracts/factories/Earth__factory.ts (1 hunks)
  • src/evm/contracts/factories/FeeVault__factory.ts (1 hunks)
  • src/evm/contracts/factories/IbcUtils__factory.ts (1 hunks)
  • src/evm/contracts/factories/Ibc__factory.ts (2 hunks)
  • src/evm/contracts/factories/Mars.sol/Mars__factory.ts (1 hunks)
  • src/evm/contracts/factories/Mars.sol/PanickingMars__factory.ts (1 hunks)
  • src/evm/contracts/factories/Mars.sol/RevertingBytesMars__factory.ts (1 hunks)
  • src/evm/contracts/factories/Mars.sol/RevertingEmptyMars__factory.ts (1 hunks)
  • src/evm/contracts/factories/Mars.sol/RevertingStringCloseChannelMars__factory.ts (1 hunks)
  • src/evm/contracts/factories/Mars.sol/RevertingStringMars__factory.ts (1 hunks)
  • src/evm/contracts/factories/Moon__factory.ts (1 hunks)
  • src/evm/contracts/factories/UniversalChannelHandler__factory.ts (1 hunks)
✅ Files skipped from review due to trivial changes (10)
  • package.json
  • src/evm/contracts/factories/Mars.sol/Mars__factory.ts
  • src/evm/contracts/factories/Mars.sol/RevertingBytesMars__factory.ts
  • src/evm/contracts/factories/Mars.sol/RevertingStringCloseChannelMars__factory.ts
  • src/evm/contracts/factories/Earth__factory.ts
  • src/evm/contracts/factories/FeeVault__factory.ts
  • src/evm/contracts/factories/IbcUtils__factory.ts
  • src/evm/contracts/factories/Moon__factory.ts
  • src/evm/contracts/factories/Mars.sol/PanickingMars__factory.ts
  • src/evm/contracts/factories/Mars.sol/RevertingEmptyMars__factory.ts
🔇 Additional comments (9)
src/evm/contracts/factories/Dispatcher__factory.ts (1)

2093-2093: Consider verifying replaced placeholders in the updated bytecode.
The marked line indicates that the bytecode has changed. Ensure that all placeholders are replaced properly throughout the hex string and that no unexpected placeholders remain.

Run this script to confirm that no placeholder patterns remain, ensuring safe contract deployments:

✅ Verification successful

Let me analyze the output further by checking for any other potential placeholder patterns.


Based on the search results, I can see that there are two library placeholders in the bytecode:

  1. __$d825222459c46c14afb2efe0967c30e98d$__ in Dispatcher__factory.ts
  2. __$f61eb90c6f674e787d51c07f105fa231e2$__ in UniversalChannelHandler__factory.ts

Let me verify if these are the only placeholders and if they follow the expected format.


Let me try a different approach to extract and verify the placeholders.


Based on the search results, I can now provide a definitive response about the placeholders in the bytecode:

Library placeholders are properly formatted and consistent
The bytecode contains exactly two unique library placeholders that follow the standard format:

  1. __$d825222459c46c14afb2efe0967c30e98d$__ - Used in Dispatcher__factory.ts for various function calls
  2. __$f61eb90c6f674e787d51c07f105fa231e2$__ - Used in both UniversalChannelHandler__factory.ts and Dispatcher__factory.ts

Each placeholder is 42 characters long (including the __$ prefix and $__ suffix) and contains a 40-character hexadecimal identifier, which is the correct format for Solidity library linking placeholders. The placeholders are used consistently throughout the bytecode for their respective library function calls.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Searching for any potential leftover placeholders, like __$...$__
rg '__\$[A-Za-z0-9]{1,}\$__' -A 5

Length of output: 67438


Script:

#!/bin/bash
# Search for other potential placeholder patterns like $...$, __...__
rg -e '\$[A-Za-z0-9_]+\$' -e '__[A-Za-z0-9_]+__' -A 5

Length of output: 67680


Script:

#!/bin/bash
# Extract all unique placeholder patterns from the bytecode
rg -o '__\$[A-Za-z0-9]{40}\$__' | sort -u

Length of output: 43


Script:

#!/bin/bash
# Search for placeholder patterns with surrounding context
rg -o '.{10}__\$[A-Za-z0-9]+\$__.{10}'

Length of output: 2207

src/evm/contracts/Ibc.ts (4)

77-77: Confirm function references for the newly added "bytesToAddr".
The new entry in the getFunction list adds "bytesToAddr". Verify that all related references (e.g., in encodeFunctionData or decodeFunctionResult) align with the function name precisely.


146-149: Check decodeFunctionResult indexing.
When decoding results, confirm that the decoding logic for "bytesToAddr" consistently references the correct output field (the returned address). Mismatching the index in the decode could introduce subtle bugs if the array of results is reordered or expanded.


235-236: Good addition of "bytesToAddr" as a typed contract method.
This new method shape is consistent with other typed contract methods. It should integrate seamlessly with code generation and runtime usage.


311-313: Ensure function overloads remain distinct.
Observe that "toStr(bytes32)" and "toStr(uint256)" already exist. Adding another function like "bytesToAddr" is not an overload, but confirm that no overload conflicts are triggered in TypeScript bindings or ABI files.

contracts/libs/Ibc.sol (1)

324-325: Confirm "emittingContract" derivation.
"emittingContract" is read using "bytesToAddr" from log[0]. Ensure that log[0] is guaranteed to have at least 20 bytes of address data.

src/evm/contracts/factories/CrossL2Prover__factory.ts (1)

400-400: Verify bytecode changes align with parseLog modifications

The bytecode has been updated, which indicates recompilation of the underlying contract. Since this PR focuses on address & topics parsing in parseLog, we should verify that these bytecode changes are related to those modifications.

✅ Verification successful

Based on the examination of the source code, I can now provide a final response:

Bytecode changes are consistent with parseLog implementation

The bytecode update in CrossL2Prover__factory.ts is verified to be consistent with the underlying contract implementation. The parseLog function in Ibc.sol library correctly handles address and topics parsing from RLP-encoded receipt data, with proper byte slicing and RLP decoding logic. The factory bytecode reflects the complete contract implementation including this parsing functionality.

Key observations:

  • The parseLog implementation properly handles RLP decoding of receipt logs
  • Address parsing uses correct byte slicing (20 bytes) via assembly
  • Topics are properly extracted as bytes arrays from RLP encoded data
  • The contract factory bytecode includes all this functionality
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the bytecode changes are related to parseLog modifications
# Look for parseLog related functions in the source contract
ast-grep --pattern 'function parseLog($$$) {
  $$$
}'

# Look for address parsing related code
ast-grep --pattern 'function $name($$$) {
  $$$
  address $$$
  $$$
}'

Length of output: 128


Script:

#!/bin/bash
# Let's try a different approach to find parseLog and address parsing related changes

# Search for parseLog in the codebase
rg -l "parseLog" 

# Search for files containing both "address" and "topics" keywords
rg -l "address.*topics|topics.*address"

# Look for contract interface definitions
fd -e sol -e ts -x rg -l "interface.*CrossL2Prover"

# Check git history for the specific file
git log -p -- src/evm/contracts/factories/CrossL2Prover__factory.ts | head -n 50

Length of output: 678


Script:

#!/bin/bash
# Let's examine the CrossL2Prover contract implementation and interface to understand the bytecode changes

# Check the CrossL2Prover contract implementation
cat contracts/core/proofAPI/CrossL2Prover.sol

# Check the interface definition
cat contracts/interfaces/ICrossL2Prover.sol

# Look for recent changes in these files
git log -p -- contracts/core/proofAPI/CrossL2Prover.sol contracts/interfaces/ICrossL2Prover.sol | head -n 100

Length of output: 10801


Script:

#!/bin/bash
# Let's check the Ibc library that handles the parseLog functionality

# Check the Ibc library implementation
cat contracts/libs/Ibc.sol

# Look for any recent changes in the Ibc library
git log -p -- contracts/libs/Ibc.sol | head -n 100

# Check if there are any test files that verify the parseLog functionality
rg -l "parseLog.*test" 

Length of output: 12364

src/evm/contracts/factories/Mars.sol/RevertingStringMars__factory.ts (1)

899-899: Verify string handling changes in bytecode

The bytecode update in RevertingStringMars contract may be related to improvements in string handling for topics. Let's verify these changes.

✅ Verification successful

Bytecode update is consistent with contract's string reversion functionality

The bytecode update in RevertingStringMars__factory.ts is verified to be correct. The source code shows that RevertingStringMars is a test contract specifically designed to revert with string messages in its IBC callbacks:

  • onRecvPacket: Reverts with "on recv packet is reverting"
  • onChanOpenAck: Reverts with "connect ibc channel is reverting"
  • onAcknowledgementPacket: Reverts with "acknowledgement packet is reverting"

The test file confirms these behaviors are expected and tested, particularly in test_recv_packet_callback_revert_and_panic() and test_ack_packet_dapp_revert().

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for string handling and topic parsing related changes
# Look for string handling functions
ast-grep --pattern 'function $name($$$) {
  $$$
  string $$$
  $$$
}'

# Look for topic parsing related code
ast-grep --pattern 'function $name($$$) {
  $$$
  bytes[] $$$topics$$$
  $$$
}'

Length of output: 153


Script:

#!/bin/bash
# Search for files containing "RevertingStringMars" to understand the context
rg -l "RevertingStringMars"

# Look for the source contract file that generates this bytecode
fd -e sol Mars

# Look for any test files related to this contract
fd -e ts -e js Mars

Length of output: 1227


Script:

#!/bin/bash
# Check the source contract to understand the string reversion logic
cat contracts/examples/Mars.sol

# Check the test file to understand the expected behavior
cat test/Dispatcher/Dispatcher.dappHandlerRevert.t.sol

Length of output: 20395

src/evm/contracts/factories/UniversalChannelHandler__factory.ts (1)

865-865: Verify IbcUtils library integration and parseLog functionality

The bytecode update includes integration with IbcUtils library. Since this PR focuses on parseLog modifications, we should verify the library integration and parseLog functionality.

✅ Verification successful

Let me gather more information about the IbcUtils library implementation to verify the parseLog functionality.


Let me check the actual implementation of the parseLog function in the Ibc library since we found a reference to it.


Based on the search results, I can now generate the final response. The evidence shows that the parseLog function has moved from IbcUtils to the Ibc library, and the UniversalChannelHandler correctly imports and uses this updated structure. The bytecode update in the factory reflects this architectural change.

Library integration is correctly maintained

The bytecode update in UniversalChannelHandler__factory.ts is consistent with the codebase structure where:

  • The parseLog function is now part of the Ibc library (contracts/libs/Ibc.sol)
  • The UniversalChannelHandler correctly imports both Ibc and IbcUtils libraries
  • The factory's bytecode includes proper library linking references
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check IbcUtils library integration and parseLog functionality
# Look for IbcUtils library usage
ast-grep --pattern 'library IbcUtils {
  $$$
  function parseLog($$$) {
    $$$
  }
  $$$
}'

# Check for library linking references
rg -A 5 "contracts/libs/IbcUtils.sol:IbcUtils"

Length of output: 2790


Script:

#!/bin/bash
# Search for IbcUtils library implementation and parseLog function
fd -e sol -x cat {} | ast-grep --pattern 'library IbcUtils {
  $$$
}'

# Search for parseLog function usage in UniversalChannelHandler
rg -A 5 "parseLog" 

Length of output: 2694


Script:

#!/bin/bash
# Get the implementation of parseLog function from Ibc.sol
rg -A 20 "function parseLog.*returns.*" contracts/libs/Ibc.sol

# Check if there are any other files importing and using the Ibc library's parseLog
rg "import.*Ibc" -A 3

Length of output: 24156

src/evm/contracts/Ibc.ts Show resolved Hide resolved
contracts/libs/Ibc.sol Show resolved Hide resolved
contracts/libs/Ibc.sol Outdated Show resolved Hide resolved
@RnkSngh RnkSngh force-pushed the raunak/patch-parseLog branch from b9d478c to 0530f6c Compare December 20, 2024 01:08
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🔭 Outside diff range comments (1)
contracts/libs/Ibc.sol (1)

Line range hint 153-300: Fix the memory offset in bytesToAddr assembly block

The current implementation has a critical issue in the assembly block. When reading from a memory bytes array:

  1. First 32 bytes contain the array length
  2. Actual data starts at offset 32

The current add(a, 20) skips only 20 bytes, which means it's reading from the middle of the length field.

Apply this fix:

 function bytesToAddr(bytes memory a) public pure returns (address addr) {
     if (a.length != 20) {
         revert invalidAddressBytes();
     }
     assembly {
-        addr := mload(add(a, 20))
+        addr := mload(add(a, 32))
     }
 }

Additionally, consider making the error more descriptive:

-error invalidAddressBytes();
+error invalidAddressBytes(uint256 length);

And update the revert:

-revert invalidAddressBytes();
+revert invalidAddressBytes(a.length);
🧹 Nitpick comments (2)
contracts/libs/Ibc.sol (1)

335-338: Add validation for topics array

While the array initialization is correct, consider adding validation:

  1. Maximum number of topics (to prevent excessive memory allocation)
  2. Individual topic length validation
+        if (encodedTopics.length > 4) {
+            revert TooManyTopics(encodedTopics.length);
+        }
         topics = new bytes[](encodedTopics.length);
         for (uint256 i = 0; i < encodedTopics.length; i++) {
+            bytes memory topic = RLPReader.readBytes(encodedTopics[i]);
+            if (topic.length != 32) {
+                revert InvalidTopicLength(i, topic.length);
+            }
-            topics[i] = RLPReader.readBytes(encodedTopics[i]);
+            topics[i] = topic;
         }

Add new error types:

+    error TooManyTopics(uint256 count);
+    error InvalidTopicLength(uint256 index, uint256 length);
src/evm/contracts/factories/Dispatcher__factory.ts (1)

Line range hint 1423-1449: Enhance library address validation

The linkBytecode function should validate the provided library addresses before performing the replacement.

Consider adding address validation:

  static linkBytecode(
    linkLibraryAddresses: DispatcherLibraryAddresses
  ): string {
+   // Validate library addresses
+   const validateAddress = (addr: string) => {
+     if (!/^0x[0-9a-fA-F]{40}$/.test(addr)) {
+       throw new Error(`Invalid library address format: ${addr}`);
+     }
+   };
+   validateAddress(linkLibraryAddresses["contracts/libs/Ibc.sol:Ibc"]);
+   validateAddress(linkLibraryAddresses["contracts/libs/IbcUtils.sol:IbcUtils"]);
+
    let linkedBytecode = _bytecode;
    // ... rest of the function
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b9d478c and 0530f6c.

📒 Files selected for processing (18)
  • bindings/go/crossl2prover/CrossL2Prover.go (1 hunks)
  • contracts/libs/Ibc.sol (3 hunks)
  • package.json (1 hunks)
  • src/evm/contracts/Ibc.ts (5 hunks)
  • src/evm/contracts/factories/CrossL2Prover__factory.ts (1 hunks)
  • src/evm/contracts/factories/Dispatcher__factory.ts (1 hunks)
  • src/evm/contracts/factories/Earth__factory.ts (1 hunks)
  • src/evm/contracts/factories/FeeVault__factory.ts (1 hunks)
  • src/evm/contracts/factories/IbcUtils__factory.ts (1 hunks)
  • src/evm/contracts/factories/Ibc__factory.ts (2 hunks)
  • src/evm/contracts/factories/Mars.sol/Mars__factory.ts (1 hunks)
  • src/evm/contracts/factories/Mars.sol/PanickingMars__factory.ts (1 hunks)
  • src/evm/contracts/factories/Mars.sol/RevertingBytesMars__factory.ts (1 hunks)
  • src/evm/contracts/factories/Mars.sol/RevertingEmptyMars__factory.ts (1 hunks)
  • src/evm/contracts/factories/Mars.sol/RevertingStringCloseChannelMars__factory.ts (1 hunks)
  • src/evm/contracts/factories/Mars.sol/RevertingStringMars__factory.ts (1 hunks)
  • src/evm/contracts/factories/Moon__factory.ts (1 hunks)
  • src/evm/contracts/factories/UniversalChannelHandler__factory.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
  • package.json
  • src/evm/contracts/factories/IbcUtils__factory.ts
  • src/evm/contracts/factories/FeeVault__factory.ts
  • src/evm/contracts/factories/Mars.sol/Mars__factory.ts
  • src/evm/contracts/factories/Earth__factory.ts
  • src/evm/contracts/factories/Mars.sol/RevertingStringMars__factory.ts
  • src/evm/contracts/factories/Mars.sol/RevertingStringCloseChannelMars__factory.ts
  • src/evm/contracts/factories/Mars.sol/PanickingMars__factory.ts
  • src/evm/contracts/factories/Mars.sol/RevertingBytesMars__factory.ts
  • src/evm/contracts/factories/Moon__factory.ts
  • src/evm/contracts/factories/Mars.sol/RevertingEmptyMars__factory.ts
  • src/evm/contracts/Ibc.ts
🔇 Additional comments (12)
src/evm/contracts/factories/UniversalChannelHandler__factory.ts (2)

Line range hint 866-874: LGTM: Library linking implementation is correct.

The implementation correctly:

  1. Handles library address format by removing '0x' prefix
  2. Normalizes addresses to lowercase
  3. Uses proper placeholder replacement for library linking

865-865: Verify bytecode changes and test coverage.

The contract bytecode has been updated. Please ensure:

  1. The changes are properly tested
  2. The test coverage includes all modified functionality
  3. The changes are documented in the contract's documentation
✅ Verification successful

Based on the documentation and contract implementation found, I can now provide a final response:

Bytecode changes appear to be part of normal contract upgrades

The bytecode changes are consistent with the contract's documented functionality and architecture:

  1. The contract is an upgradeable contract (implements upgradeTo and upgradeToAndCall)
  2. The contract is well documented in docs/universal-channel-middleware.md
  3. The changes are related to the Universal Channel Handler which is a core middleware component for IBC packet handling

The documentation confirms this is expected behavior as:

  • The contract is designed to be upgradeable to support middleware extensions
  • It handles critical IBC packet routing and channel management
  • The implementation matches the documented interface and functionality
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify test coverage for UniversalChannelHandler contract

# Check for test files
fd -e ts -e js "UniversalChannelHandler.*test" 

# Check for documentation updates
fd -e md "UniversalChannelHandler"

Length of output: 83


Script:

#!/bin/bash
# Let's try a broader search to find test files and documentation

# Search for test files using ripgrep
echo "=== Searching for test files ==="
rg -l "UniversalChannelHandler" --type ts --type js | grep -i "test"

# Search for documentation files
echo -e "\n=== Searching for documentation files ==="
rg -l "UniversalChannelHandler" --type md

# Search for imports of UniversalChannelHandler to find related files
echo -e "\n=== Searching for related files ==="
rg -l "UniversalChannelHandler" --type ts --type js | grep -v "test" | grep -v "factories"

# Check if there are any recent git commits related to this contract
echo -e "\n=== Recent related commits ==="
git log -n 5 --oneline --grep="UniversalChannelHandler"

Length of output: 688


Script:

#!/bin/bash
# Let's check the documentation and contract implementation for details about the changes

# Check the documentation content
echo "=== Documentation Content ==="
cat docs/universal-channel-middleware.md

# Check the contract interface
echo -e "\n=== Contract Interface ==="
cat src/evm/contracts/IUniversalChannelHandler.ts

# Check the contract implementation
echo -e "\n=== Contract Implementation ==="
cat src/evm/contracts/UniversalChannelHandler.ts

# Look for test files in a different location
echo -e "\n=== Looking for tests in other locations ==="
fd -e ts -e js "test" | grep -i "channel"

Length of output: 43592

src/evm/contracts/factories/Ibc__factory.ts (1)

121-139: LGTM - Generated code matches Solidity changes

The TypeScript bindings have been correctly generated to include:

  1. New bytesToAddr function definition
  2. New invalidAddressBytes error type

Also applies to: 519-523

bindings/go/crossl2prover/CrossL2Prover.go (1)

54-54: LGTM - Generated code matches Solidity changes

The Go bindings have been correctly generated to include the new invalidAddressBytes error type in the ABI.

src/evm/contracts/factories/CrossL2Prover__factory.ts (2)

397-401: LGTM: New error type for address validation.

The addition of invalidAddressBytes error type aligns with the new address validation functionality in the contract.

Let's verify the error usage in the contract implementation:

✅ Verification successful

Let me gather more context about the error usage by checking the surrounding code:


Verified: The invalidAddressBytes error is properly implemented and used

The error is correctly used in the bytesToAddr function within Ibc.sol to validate address byte length. Its presence in the factory files and bindings is consistent with the contract implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of invalidAddressBytes error in the contract implementation

# Search for error declaration
ast-grep --pattern 'error invalidAddressBytes()'

# Search for error usage
rg 'revert\s+invalidAddressBytes'

Length of output: 151


Script:

#!/bin/bash
# Get the context around the error usage
rg -B 5 -A 5 'revert\s+invalidAddressBytes' contracts/libs/Ibc.sol

# Check if this error is used in any other files
rg --files-with-matches 'invalidAddressBytes'

Length of output: 551


405-405: Verify bytecode consistency.

The bytecode has been updated, which is expected due to the contract implementation changes.

Let's verify the bytecode consistency:

src/evm/contracts/factories/Dispatcher__factory.ts (6)

Line range hint 1-1396: LGTM: Contract ABI is well-structured

The contract ABI is comprehensive and well-structured, including all necessary function signatures, events, and error definitions.


Line range hint 1397-1422: LGTM: Factory constructor implementation is type-safe

The factory constructor properly handles both direct bytecode initialization and library linking scenarios using TypeScript's type system.


Line range hint 1450-1489: LGTM: Factory methods are properly typed

The factory methods (getDeployTransaction, deploy, connect) are well-implemented with proper TypeScript types and return values.


Line range hint 1490-1494: LGTM: Interface creation and contract connection methods

The static methods for interface creation and contract connection are properly implemented.


Line range hint 1495-1498: LGTM: Library addresses interface is well-defined

The DispatcherLibraryAddresses interface properly defines the required library addresses.


2093-2093: Verify the bytecode update

The bytecode has been updated. This change should be verified to ensure it matches the compiled contract output.

✅ Verification successful

Bytecode update appears to be valid

The bytecode update appears to be valid based on the following evidence:

  1. The same bytecode prefix appears in both Dispatcher and UniversalChannelHandler factories, which is expected as they share common initialization patterns from OpenZeppelin's upgradeable contracts.
  2. The contracts have mutual dependencies as shown by the cross-references in their source code.
  3. No recent changes were detected in the git history for this bytecode or the contract source, suggesting this is a stable, established implementation.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify if the bytecode matches the compiled contract output
# Note: This assumes the contract has been compiled and artifacts are available

# Search for any Dispatcher.json files that might contain the compiled bytecode
fd -e json -E 'node_modules' 'Dispatcher.json' | while read -r file; do
  echo "Checking $file"
  jq -r '.bytecode' "$file"
done

Length of output: 59


Script:

#!/bin/bash
# Let's try a different approach to verify the bytecode by:
# 1. Looking for any Solidity contract files that might be the source
# 2. Looking for any other references to this bytecode

# Search for Dispatcher contract source files
echo "=== Searching for Dispatcher contract files ==="
fd -t f -e sol 'Dispatcher.sol'

# Search for other factory files that might reference this bytecode
echo -e "\n=== Searching for other factory files ==="
fd -t f -e ts 'factory' -E 'node_modules'

# Search for any files containing part of this specific bytecode
echo -e "\n=== Searching for bytecode references ==="
# Using a unique portion of the bytecode to avoid false positives
rg -l "0x60a0604052306080523480156200001557600080fd" --type ts

Length of output: 2667


Script:

#!/bin/bash
# Let's analyze the source contract and compare bytecode references

# First, let's check the Dispatcher contract source
echo "=== Dispatcher.sol content ==="
cat contracts/core/Dispatcher.sol

# Check if the same bytecode appears in UniversalChannelHandler factory
echo -e "\n=== UniversalChannelHandler factory bytecode ==="
rg -A 2 -B 2 "0x60a0604052306080523480156200001557600080fd" src/evm/contracts/factories/UniversalChannelHandler__factory.ts

# Look for any recent changes to these files
echo -e "\n=== Recent changes to related files ==="
git log -p --since="1 month ago" -- contracts/core/Dispatcher.sol src/evm/contracts/factories/Dispatcher__factory.ts src/evm/contracts/factories/UniversalChannelHandler__factory.ts

Length of output: 62631


Script:

#!/bin/bash
# Let's analyze the bytecode differences and contract dependencies

# Check if there are any recent changes to the bytecode
echo "=== Checking recent bytecode changes ==="
git log -p --since="1 month ago" -S"0x60a0604052306080523480156200001557600080fd" src/evm/contracts/factories/Dispatcher__factory.ts src/evm/contracts/factories/UniversalChannelHandler__factory.ts

# Look for any dependencies between these contracts
echo -e "\n=== Checking contract dependencies ==="
rg -l "UniversalChannelHandler" contracts/core/Dispatcher.sol
rg -l "Dispatcher" contracts/core/UniversalChannelHandler.sol 2>/dev/null || echo "UniversalChannelHandler.sol not found"

# Check if there are any recent changes to the contract source
echo -e "\n=== Checking recent contract changes ==="
git log -p --since="1 month ago" contracts/core/Dispatcher.sol

Length of output: 477

contracts/libs/Ibc.sol Show resolved Hide resolved
@RnkSngh RnkSngh merged commit 2c480fa into main Dec 20, 2024
3 checks passed
@RnkSngh RnkSngh deleted the raunak/patch-parseLog branch December 20, 2024 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants