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

fix typo in chainId -> chainID in crossl2 prover #293

Merged
merged 1 commit into from
Dec 18, 2024
Merged

Conversation

RnkSngh
Copy link
Collaborator

@RnkSngh RnkSngh commented Dec 18, 2024

i mistyped chainID as chainId and then compiler didn't complain when they were different types because they were different variables 😱

Summary by CodeRabbit

  • New Features

    • Updated return types for validateEvent and validateReceipt functions to use string instead of bytes32 for chainId and srcChainId, respectively.
    • Removed the counterParty variable from the Venus contract, simplifying its state and constructor parameters.
    • Added an expectedEmitter parameter to the receiveTransmissionEvent function for enhanced validation.
  • Bug Fixes

    • Adjusted output structures to ensure consistency across contract interactions.
  • Chores

    • Updated version number in package.json from 4.0.17 to 4.0.18.

Copy link

coderabbitai bot commented Dec 18, 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/CrossL2Prover.ts

(node:10437) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///eslint.config.js?mtime=1734521317918 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 significant changes to multiple contracts and their Go and TypeScript bindings, primarily focusing on modifying the data types for chain identifiers from bytes32 to string. The changes span across several files in the CrossL2Prover and Venus contracts, updating function signatures, event definitions, and contract interfaces. The modifications aim to standardize the representation of chain identifiers and simplify contract interactions by removing the counterParty concept.

Changes

File Change Summary
bindings/go/crossl2prover/CrossL2Prover.go Updated ABI definitions, changed validateEvent and validateReceipt return types from bytes32 to string
bindings/go/icrossl2prover/ICrossL2Prover.go Modified method output types for validateEvent and validateReceipt
bindings/go/venus/Venus.go Removed CounterParty methods, updated ChainId and ReceiveTransmissionEvent method signatures
contracts/core/proofAPI/CrossL2Prover.sol Changed validateReceipt and validateEvent return types from bytes32 to string
contracts/examples/Venus.sol Removed counterParty, changed chainId type to string, updated constructor and event signatures
contracts/interfaces/ICrossL2Prover.sol Updated validateReceipt and validateEvent method signatures
package.json Version bumped from 4.0.17 to 4.0.18
src/evm/* Updated contract interfaces and factories to reflect type changes

Possibly related PRs

Suggested Reviewers

  • dshiell
  • alfredo-stonk

Poem

🐰 Bytes to strings, a metamorphosis divine,
Chain IDs dancing in a new design,
Contracts evolve with graceful might,
Simplifying paths, making logic light!
A rabbit's code-hop of pure delight! 🌈

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: 1

🔭 Outside diff range comments (2)
bindings/go/crossl2prover/CrossL2Prover.go (1)

Inconsistency found in interface documentation

The interface file contracts/interfaces/ICrossL2Prover.sol has a documentation comment that uses "chainID" while the actual function signature uses "chainId". The comment should be updated to maintain consistency with the implementation.

  • contracts/interfaces/ICrossL2Prover.sol: Update the @return chainId documentation to use "chainId" instead of "chainID" in the description.
🔗 Analysis chain

Line range hint 329-349: Confirm correct usage of 'chainId' instead of 'chainID'.

The function's Solidity signature and Go binding now use "chainId" consistently. This matches the PR objective of aligning naming. Ensure other parts of the codebase that consume this method are updated to the new name, if any references existed to the older "chainID".

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify no references to 'chainID' remain in the repo
rg 'chainID'

Length of output: 121


Script:

#!/bin/bash
# Let's check the full context of the interface definition to understand if this is a comment or actual code
rg -A 10 'chainId The chainID' contracts/interfaces/ICrossL2Prover.sol

Length of output: 764

src/evm/contracts/Venus.ts (1)

Line range hint 85-91: Type change: BytesLike to string for chain IDs

The type change from BytesLike to string for srcChainId could affect data validation and encoding. Ensure that:

  1. All consumers can handle string chain IDs
  2. The string format is well-defined and validated
  3. No data loss occurs during type conversion

Consider adding input validation for the string format to prevent invalid chain IDs.

🧹 Nitpick comments (2)
bindings/go/crossl2prover/CrossL2Prover.go (1)

384-396: Check for potential string-based vulnerabilities or large data usage.

While changing from bytes32 to string for chain identifiers can be more readable or flexible, it may lead to higher gas usage when storing or handling chain IDs. Consider whether a length-limited approach or standardized representation (e.g., a struct or fixed-length bytes) might be more optimal.

contracts/examples/Venus.sol (1)

89-91: Review string comparison implementation

The use of Bytes.equal for string comparison is correct but consider adding a helper function for chain ID comparison if this pattern is used frequently.

+    function compareChainIds(string memory a, string memory b) internal pure returns (bool) {
+        return Bytes.equal(bytes(a), bytes(b));
+    }

Also applies to: 120-121

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 4a0a91a and 07fe378.

📒 Files selected for processing (12)
  • bindings/go/crossl2prover/CrossL2Prover.go (7 hunks)
  • bindings/go/icrossl2prover/ICrossL2Prover.go (8 hunks)
  • bindings/go/venus/Venus.go (6 hunks)
  • contracts/core/proofAPI/CrossL2Prover.sol (2 hunks)
  • contracts/examples/Venus.sol (5 hunks)
  • contracts/interfaces/ICrossL2Prover.sol (2 hunks)
  • package.json (1 hunks)
  • src/evm/contracts/CrossL2Prover.ts (2 hunks)
  • src/evm/contracts/Venus.ts (5 hunks)
  • src/evm/contracts/factories/CrossL2Prover__factory.ts (3 hunks)
  • src/evm/contracts/factories/ICrossL2Prover__factory.ts (2 hunks)
  • src/evm/contracts/factories/Venus__factory.ts (6 hunks)
✅ Files skipped from review due to trivial changes (1)
  • package.json
🔇 Additional comments (19)
bindings/go/crossl2prover/CrossL2Prover.go (1)

54-54: ABI consistency check.

The ABI string at line 54 reflects the updated return types (e.g., "chainId" as a string) for the contract methods. This matches the rest of the code changes in this file, ensuring the Go bindings are correctly synchronized with Solidity.

bindings/go/venus/Venus.go (4)

34-34: ABI alignment for 'chainId' representation.

At line 34, the ABI references to "_chainId" as a string are correctly reflected in the rest of the methods. No leftover references to "chainID" detected here.


185-210: String-based chainId return type.

The functions at lines 185-210 consistently return chainId as a string. This change is in line with the updated interface. Confirm that any client code dealing with chain ID values still works correctly now that it’s a dynamic string rather than a fixed type.


318-336: Extended parameters for 'receiveTransmissionEvent'.

The addition of the 'expectedEmitter' address parameter at lines 318-336 replaces the removed “counterParty” usage. Confirm upstream calls consistently provide the new parameter.
[approve]


408-408: Event log updates to string-based SrcChainId.

The "SuccessfulReceipt" event now includes "srcChainId" as a string at line 408. All filter/watch/parse methods at lines 413+, 462+ are updated accordingly. This is consistent with the broader shift away from bytes32.

Also applies to: 413-427, 462-464

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

85-86: Renamed output to string-based chainId.

The outputs now reflect "chainId" as "string" instead of bytes32. This fix implements the PR objective of correcting the variable name and type.


119-120: Changed srcChainId field to string.

The shift from bytes32 → string for srcChainId here is consistent with the CrossL2Prover changes. Confirm no leftover references to the old type remain in the TypeScript codebase.

✅ Verification successful

Let me search for more specific references to srcChainId in TypeScript files.


Let me try one more search to be thorough, using a different ripgrep approach:


Type change from bytes32 to string is consistently applied

Based on the comprehensive search results, all references to srcChainId in the TypeScript codebase are using the string type. The change has been properly propagated across all relevant files including Venus.ts, ICrossL2Prover.ts, and their respective factory files. No leftover references to the old bytes32 type were found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Checks for references to 'srcChainId' as bytes32 in TS code
rg 'srcChainId.*bytes32'

Length of output: 3434


Script:

#!/bin/bash
# Search for srcChainId in TypeScript files specifically
rg -t typescript 'srcChainId'

# Also check for any interface or type definitions that might reference srcChainId
ast-grep --pattern 'interface $_ {
  $$$
  srcChainId
  $$$
}'

Length of output: 335


Script:

#!/bin/bash
# Search in all files with common TypeScript/JavaScript extensions
rg -g '*.{ts,tsx,js,jsx}' 'srcChainId'

# Also check for any type annotations with bytes32
rg -g '*.{ts,tsx}' 'srcChainId.*:.*bytes'

Length of output: 696

contracts/interfaces/ICrossL2Prover.sol (2)

69-69: Changed return type for srcChainId in validateReceipt.

Returning srcChainId as a string aligns with the rest of the codebase updates. Make sure any domain logic expecting a 32-byte value is updated accordingly.


87-87: Renamed chainId in validateEvent.

The function’s return type is now “string memory chainId,” fixing the mismatch. This similarly addresses the original issue of chainID vs chainId.

contracts/core/proofAPI/CrossL2Prover.sol (2)

Line range hint 84-88: Ensure consistent chain ID validation across the codebase

The change to string type for chain IDs should be consistently applied across all contracts and interfaces.


Line range hint 50-78: Consider the performance implications of using string for chain IDs

While changing from bytes32 to string improves readability, it increases gas costs due to dynamic memory allocation and string comparison operations. Consider documenting any performance impact and standardizing the chain ID format.

contracts/examples/Venus.sol (2)

32-32: Verify the impact of removing counterParty variable

The removal of the counterParty variable and change to string chainId requires careful verification of all dependent contracts.

Also applies to: 40-40


114-124: Validate expectedEmitter parameter thoroughly

The addition of expectedEmitter parameter improves security but ensure all calling contracts are updated accordingly.

✅ Verification successful

Let me search for any actual implementations or calls to this function in the codebase to ensure we have a complete picture.


Let me check the content of these test files to see how the function is being used.


Let me try a broader search to find any references to this function, including potential interface definitions or inheritance.


Based on the search results, I can now provide the final response:

No external callers found - parameter addition is safe

The receiveTransmissionEvent function appears to be newly introduced or only defined in the Venus contract itself, with no existing calls found in the Solidity codebase. The Go bindings are auto-generated and will be updated accordingly. The addition of the expectedEmitter parameter is safe to proceed with.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for existing calls to receiveTransmissionEvent
rg -A 2 'receiveTransmissionEvent\('

Length of output: 1846


Script:

#!/bin/bash
# Search for actual calls or implementations using this function
rg -l "\.receiveTransmissionEvent\(" --type-add 'sol:*.sol' --type sol

# Also check for any test files that might be using this function
fd -e sol -e js -e ts test

Length of output: 164


Script:

#!/bin/bash
# Check the test files for usage of receiveTransmissionEvent
rg -A 5 "receiveTransmissionEvent" test/utils/TestUtils.t.sol src/scripts/fork-deployment-test.ts

Length of output: 97


Script:

#!/bin/bash
# Look for any mentions of receiveTransmissionEvent in the entire codebase
rg "receiveTransmissionEvent" --type-add 'sol:*.sol' --type sol -B 2 -A 2

# Also check if Venus contract inherits from any interfaces
ast-grep --pattern 'contract Venus is $_' 

Length of output: 645

src/evm/contracts/CrossL2Prover.ts (1)

221-221: Ensure TypeScript bindings are updated consistently

The return type changes in the TypeScript interface match the Solidity contract changes. Verify that all consuming TypeScript code is updated accordingly.

Also applies to: 287-287

✅ Verification successful

No direct usage of validateReceipt method found

Based on the search results, the validateReceipt method appears only in type definitions and contract interfaces (CrossL2Prover.ts, ICrossL2Prover.ts, and their respective factories). There are no direct calls to this method in the TypeScript codebase, suggesting it's likely only used through contract interactions at runtime.

The type changes are properly reflected in both the main contract interface and its interface counterpart, maintaining consistency in the TypeScript bindings.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for validateReceipt usage in TypeScript files
rg -t ts -A 2 'validateReceipt\('

Length of output: 35


Script:

#!/bin/bash
# Search for validateReceipt usage with a broader pattern
rg -t ts "validateReceipt"

# Also search for any imports of CrossL2Prover to find potential consumers
rg -t ts "CrossL2Prover"

# Search for the class definition to understand the context better
ast-grep --pattern 'class CrossL2Prover {
  $$$
}'

Length of output: 4533

src/evm/contracts/Venus.ts (2)

Line range hint 281-287: Event signature updated to match type changes

The event filter signature has been updated to use string type for chain IDs. This change is consistent with the type changes above.


61-61: ⚠️ Potential issue

Breaking change: New parameter added to receiveTransmissionEvent

The function signature has been updated to include an expectedEmitter parameter. This is a breaking change that will require updates to all callers of this function.

Also applies to: 209-209, 246-246

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

280-283: ⚠️ Potential issue

Breaking changes in contract deployment

Multiple breaking changes have been introduced:

  1. The _chainId parameter type has changed from BytesLike to string
  2. The _counterParty parameter has been removed

These changes will affect contract deployment scripts and factory usage.

Also applies to: 287-290

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

141-142: Verify chain identifier type consistency across the codebase.

The change from bytes32 to string for the chainId output type in validateEvent aligns with the PR objectives. However, we should verify this change is consistently applied across related files.

✅ Verification successful

Based on the search results, I can generate the final response as we have sufficient information to verify the chain identifier type consistency. The search results show:

Chain identifier type changes are consistently applied across the codebase

The change from bytes32 to string for chain identifiers is consistently implemented across the relevant interfaces and implementations:

  • In contracts/interfaces/ICrossL2Prover.sol, both validateEvent and validateReceipt functions use string type
  • In contracts/examples/Venus.sol, the chainId field and event parameters use string type
  • In contracts/core/proofAPI/CrossL2Prover.sol, the implementation uses string type
  • The only remaining bytes32 chainId usage is in SequencerSignatureVerifier.sol which is a separate component with different requirements
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify chain identifier type consistency across related files

# Check for any remaining bytes32 chainId references
rg -i 'chainid.*bytes32|bytes32.*chainid'

# Check for string chainId declarations
rg -i 'chainid.*string|string.*chainid'

Length of output: 29148


174-176: Verify validateReceipt return type changes.

The changes to validateReceipt include:

  1. Changed first return parameter type from bytes32 to string
  2. Removed parameter name receiptRLP from second return value

These changes align with the PR objectives for standardizing chain identifiers.

Also applies to: 179-179

bindings/go/icrossl2prover/ICrossL2Prover.go Show resolved Hide resolved
@RnkSngh RnkSngh merged commit edc2503 into main Dec 18, 2024
3 checks passed
@RnkSngh RnkSngh deleted the raunak/fixtypo branch December 18, 2024 11:32
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