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

update launch command; adding celestia codec, several bug fix #116

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

sh-cha
Copy link
Contributor

@sh-cha sh-cha commented Oct 21, 2024

Description

Closes: #XXXX


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...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers 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...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Enhanced address derivation functionality with new methods for L1 and L2 addresses.
    • Introduced a new Bech32 codec for Celestia addresses.
    • Added methods for updating bridge configurations, including proposer, challenger, and parameters.
    • Improved the genesis state initialization process with better error handling and logging.
  • Bug Fixes

    • Improved error handling and logging during genesis state initialization.
  • Documentation

    • Updated configuration settings in the Launch Tools documentation to reflect recent changes.
    • Enhanced the README to clarify the purpose and functionality of the OPinit Stack.
  • Chores

    • Minor renaming of output file paths for clarity.

@sh-cha sh-cha self-assigned this Oct 21, 2024
@sh-cha sh-cha requested a review from a team as a code owner October 21, 2024 09:01
Copy link

coderabbitai bot commented Oct 21, 2024

Walkthrough

The changes in this pull request involve significant updates to the configuration settings and functionality of the Launch Tools within the contrib/launchtools directory. Key modifications include adjustments to the README.md configuration parameters, enhancements to address derivation methods in config.go, and improvements in error handling and logging in genesis.go. Additionally, a new codec function for Celestia addresses has been introduced in address.go, and a minor output message change has been made in cmd.go. These changes collectively aim to refine the configuration management and initialization processes.

Changes

File Path Change Summary
contrib/launchtools/README.md Updated l1_config and op_bridge settings, added enable_oracle, and replaced address fields with l1_address and l2_address. Removed entries in genesis_accounts.
contrib/launchtools/cmd.go Renamed output message from "artifact.json" to "artifacts.json" in LaunchCmd function.
contrib/launchtools/config.go Added functions for address derivation, updated Finalize method to accept batchSubmissionTarget, and improved error handling.
contrib/launchtools/steps/genesis.go Enhanced error handling and logging, modified addFeeWhitelists, introduced setMultiOpChildBridgeExecutorsAddress, and refined genesis state preparation.
contrib/launchtools/utils/address.go Added CelestiaAddressCodec function for Bech32 codec initialization, along with address derivation functions.
x/ophost/keeper/msg_server.go Added multiple methods to MsgServer for updating bridge configurations, including error handling and event emissions.

Possibly related PRs

🐇 In the launch tools, we hop and play,
With configs and addresses, we pave the way.
From initiation-1 to initiation-2,
New paths and codecs, oh what a view!
With logs that sparkle and errors that fade,
Our genesis shines, in the code parade! 🌟


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

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.61%. Comparing base (30fefaa) to head (e6ea82d).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #116   +/-   ##
=======================================
  Coverage   48.61%   48.61%           
=======================================
  Files          57       57           
  Lines        4229     4229           
=======================================
  Hits         2056     2056           
  Misses       1744     1744           
  Partials      429      429           
Files with missing lines Coverage Δ
x/ophost/keeper/msg_server.go 56.60% <ø> (ø)

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

🧹 Outside diff range and nitpick comments (2)
contrib/launchtools/utils/address.go (1)

17-19: LGTM! Consider adding a comment for consistency.

The new CelestiaAddressCodec function is correctly implemented and follows the existing pattern in the file. It aligns with the PR objective of incorporating the Celestia codec.

For consistency with the L1AddressCodec function, consider adding a brief comment explaining the purpose of this function:

+// CelestiaAddressCodec returns a new Bech32 codec for Celestia addresses
 func CelestiaAddressCodec() address.Codec {
 	return authcodec.NewBech32Codec("celestia")
 }
contrib/launchtools/cmd.go (1)

Line range hint 26-115: Consider refactoring for improved readability and maintainability.

While the LaunchCmd function is well-structured, consider the following improvements:

  1. Break down the function into smaller, more focused functions to improve readability and maintainability.
  2. Use more specific error messages to aid in debugging.
  3. Consider using a proper logging framework instead of fmt.Fprintf for error output.

Here's a high-level example of how you might refactor this function:

func LaunchCmd(...) *cobra.Command {
    cmd := &cobra.Command{
        // ... (command details)
        RunE: runLaunchCmd,
    }
    // ... (flag setup)
    return cmd
}

func runLaunchCmd(cmd *cobra.Command, args []string) error {
    config, err := setupConfig(cmd)
    if err != nil {
        return err
    }

    launcher, err := setupLauncher(cmd, config)
    if err != nil {
        return err
    }

    if err := runLauncherSteps(launcher, config); err != nil {
        return err
    }

    return outputArtifacts(cmd, launcher, config)
}

// Implement setupConfig, setupLauncher, runLauncherSteps, and outputArtifacts as separate functions

This refactoring would make the code more modular and easier to maintain.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 30fefaa and b1e4e81.

📒 Files selected for processing (5)
  • contrib/launchtools/README.md (2 hunks)
  • contrib/launchtools/cmd.go (1 hunks)
  • contrib/launchtools/config.go (9 hunks)
  • contrib/launchtools/steps/genesis.go (0 hunks)
  • contrib/launchtools/utils/address.go (1 hunks)
💤 Files with no reviewable changes (1)
  • contrib/launchtools/steps/genesis.go
🧰 Additional context used
🔇 Additional comments (16)
contrib/launchtools/utils/address.go (1)

17-19: Verify the usage of the new CelestiaAddressCodec function.

The addition of CelestiaAddressCodec expands the address codec functionality to include Celestia addresses. This aligns with the PR objectives and is a well-contained change.

To ensure this new function is being used appropriately, please run the following script to check its usage across the codebase:

This will help verify that the new function is being utilized as intended and that there are no outstanding tasks related to Celestia address handling.

✅ Verification successful

CelestiaAddressCodec Usage Verified

The CelestiaAddressCodec function is used in contrib/launchtools/config.go. No related TODOs or FIXMEs were found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the usage of CelestiaAddressCodec function

# Test: Search for CelestiaAddressCodec usage
echo "Searching for CelestiaAddressCodec usage:"
rg "CelestiaAddressCodec\(\)" --type go

# Test: Check if there are any TODOs or FIXMEs related to Celestia address handling
echo "Checking for TODOs or FIXMEs related to Celestia addresses:"
rg "TODO|FIXME" --type go | rg -i "celestia.*address"

Length of output: 469

contrib/launchtools/README.md (6)

58-61: LGTM. Verify L1-only address design for submitters.

The output_submitter and batch_submitter configurations have been updated to use l1_address instead of address. This change is consistent with the new L1/L2 address distinction.

Please confirm:

  1. The intention to have only L1 addresses for these roles.
  2. That all code referencing these addresses has been updated accordingly.
  3. Consider adding a comment explaining why these roles only require L1 addresses, if this is indeed the intended configuration.

To verify the impact and usage, run the following script:

✅ Verification successful

Verified L1-only address implementation for submitters.
No additional references to output_submitter.address or batch_submitter.address were found in the codebase, confirming that the renaming to l1_address is correctly implemented and scoped.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of output submitter and batch submitter addresses in the code
echo "Searching for output submitter address usage:"
rg "output_submitter.*address" --type go
echo "Searching for batch submitter address usage:"
rg "batch_submitter.*address" --type go

Length of output: 1673


31-32: LGTM. Verify impact on dependent systems.

The chain_id and rpc_url have been updated consistently to reflect the new chain "initiation-2". This change looks good, but it's important to ensure that all dependent systems and documentation are updated accordingly.

To verify the impact, run the following script:

✅ Verification successful

Verification Successful

No occurrences of the old chain_id ("initiation-1") or rpc_url ("https://rpc.initiation-1.initia.xyz:443") were found in the codebase. Changes are verified.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for occurrences of the old chain ID and RPC URL
echo "Searching for old chain ID (initiation-1):"
rg "initiation-1" --type md --type go --type json
echo "Searching for old RPC URL:"
rg "https://rpc.initiation-1.initia.xyz:443" --type md --type go --type json

Length of output: 284


53-54: ```shell
#!/bin/bash

Corrected search for bridge_executor address usage in Go files

echo "Searching for bridge_executor address usage in Go files:"
rg "bridge_executor.*address" --type go


---

Line range hint `67-80`: **LGTM. Verify impact on system initialization.**

The `genesis_accounts` section has been updated to remove entries for `output_submitter` and `batch_submitter`. This change is consistent with the modifications in the `system_keys` section.

Please ensure:
1. The removal of these accounts from the genesis configuration doesn't negatively impact the system's initialization or operation.
2. Any code or processes that previously relied on these accounts being present in the genesis state have been updated accordingly.
3. The documentation reflects this change in the genesis account setup.


To verify the impact, run the following script:

<details>
<summary>✅ Verification successful</summary>

<!-- [monologue]
[task finalize_verification_1/1]
Finalize the verification based on the shell script output.
[observations]
- The corrected shell script did not find any references to `output_submitter` and `batch_submitter` in genesis-related Go code.
- This indicates that their removal from the `genesis_accounts` section does not affect system initialization.
[analysis]
- The absence of references suggests that these accounts are no longer in use and their removal is safe.
- No further action is needed unless there are non-Go files that might reference these accounts, which is unlikely based on the context.
[actions]
- Conclude the verification by approving the changes and confirming that the removal does not impact system initialization.
-->
**Verified: Removal of `output_submitter` and `batch_submitter`**

No references to `output_submitter` and `batch_submitter` were found in genesis-related Go code, ensuring that their removal does not impact system initialization.



</details>

<details>
<summary>🏁 Scripts executed</summary>

The following scripts were executed for the analysis:

Script:

```shell
#!/bin/bash
# Search for references to output_submitter and batch_submitter in genesis-related code
echo "Searching for output_submitter in genesis-related code:"
rg "output_submitter" --type go | rg "genesis"
echo "Searching for batch_submitter in genesis-related code:"
rg "batch_submitter" --type go | rg "genesis"

Length of output: 1673


Script:

#!/bin/bash
# Corrected script to search for references to output_submitter and batch_submitter in genesis-related Go code

echo "Searching for output_submitter in genesis-related Go code:"
rg "output_submitter" --type go | rg "genesis"

echo "Searching for batch_submitter in genesis-related Go code:"
rg "batch_submitter" --type go | rg "genesis"

Length of output: 340


64-65: LGTM. Verify design decision for identical L1 and L2 addresses for challenger.

The challenger configuration now includes both l1_address and l2_address, similar to the bridge executor. Both addresses are currently set to the same value.

Please address the following:

  1. Confirm if having identical L1 and L2 addresses for the challenger is intentional.
  2. Ensure that all code referencing the challenger's address has been updated to use the correct address (L1 or L2) based on the context.
  3. Consider adding a comment explaining the reason for identical addresses, if this is the intended configuration.
  4. Verify if this change is consistent with the overall system design and the roles of the challenger in both L1 and L2 contexts.

To verify the impact and usage, run the following script:


41-45: LGTM. Verify new parameters and type changes.

The changes in the op_bridge section improve readability and introduce new parameters:

  1. Time intervals are now in a human-readable format (e.g., "1h0m0s", "168h0m0s").
  2. output_submission_start_height is now an integer instead of a string.
  3. New parameters batch_submission_target and enable_oracle have been added.

These changes look good, but please ensure:

  1. The code handling these parameters can parse the new time format.
  2. The type change for output_submission_start_height is handled correctly in the code.
  3. The new parameters are properly implemented and documented in the codebase.

To verify the impact, run the following script:

contrib/launchtools/cmd.go (1)

99-99: Confirm consistency of artifact file name change.

The file name in the output message has been changed from "artifact.json" to "artifacts.json". This change appears to be intentional and aligns with the PR objectives of updating the launch command.

To ensure consistency across the codebase, let's verify if this change is reflected in other relevant files:

This will help us confirm if the change has been consistently applied throughout the project.

✅ Verification successful

Artifact file name change is consistent across the codebase.

All occurrences of "artifact.json" have been successfully updated to "artifacts.json".

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for occurrences of both "artifact.json" and "artifacts.json" in the codebase.

echo "Searching for 'artifact.json':"
rg -i "artifact\.json"

echo "\nSearching for 'artifacts.json':"
rg -i "artifacts\.json"

Length of output: 425

contrib/launchtools/config.go (8)

14-14: Imports added correctly.

The new imports are necessary for the added functionality involving address codecs and cryptography.

Also applies to: 21-21


80-82: Ensure 'BatchSubmissionTarget' is properly initialized before use.

Confirm that i.OpBridge.BatchSubmissionTarget is set correctly before calling SystemKeys.Finalize to avoid potential issues with uninitialized variables.


310-313: Skipping accounts with empty L2Address is appropriate.

The added check prevents unnecessary processing of accounts without an L2Address, enhancing efficiency.


370-376: deriveAddress function updated to accept address.Codec.

This change allows address derivation to be flexible based on the provided codec, improving the function's reusability.


378-386: Implemented deriveL1L2Addresses function correctly.

The new function efficiently derives both L1 and L2 addresses from a given mnemonic.


387-399: deriveAddressWithChainType function handles chain types effectively.

The function selects the appropriate codec based on chainType, ensuring correct address derivation for different chains.


408-408: Proper error handling in deriveAddressBz.

Errors during private key derivation are correctly wrapped, and the function returns the public key address as expected.

Also applies to: 412-412


Line range hint 415-521: Updated SystemKeys.Finalize to accommodate batchSubmissionTarget.

The method now properly initializes system accounts using the new address derivation functions and the provided batch submission target.

contrib/launchtools/README.md Show resolved Hide resolved
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: 0

🧹 Outside diff range and nitpick comments (5)
x/ophost/keeper/msg_server.go (5)

Line range hint 350-388: Refactor duplicated logic in update methods to improve maintainability

The methods UpdateProposer, UpdateChallenger, UpdateBatchInfo, UpdateOracleConfig, and UpdateMetadata all share similar code structures, including validation, permission checks, configuration updates, and event emission. Refactoring common logic into helper functions or a generic update handler can reduce code duplication and enhance maintainability.

Consider creating a generic helper function to handle the common update logic:

func (ms MsgServer) updateBridgeConfig(ctx context.Context, reqAuthority string, bridgeId uint64, expectedAuthority string, updateFunc func(*types.BridgeConfig) error, eventType string, eventAttributes []sdk.Attribute) error {
    if ms.authority != reqAuthority && expectedAuthority != reqAuthority {
        return govtypes.ErrInvalidSigner.Wrapf("invalid authority; expected %s or %s, got %s", ms.authority, expectedAuthority, reqAuthority)
    }

    config, err := ms.GetBridgeConfig(ctx, bridgeId)
    if err != nil {
        return err
    }

    if err := updateFunc(&config); err != nil {
        return err
    }

    if err := ms.SetBridgeConfig(ctx, bridgeId, config); err != nil {
        return err
    }

    sdk.UnwrapSDKContext(ctx).EventManager().EmitEvent(sdk.NewEvent(
        eventType,
        eventAttributes...,
    ))

    return nil
}

Then, each update method can use this helper function, passing in the specific update logic and event details.

Also applies to: 395-434, 440-487, 489-516


Line range hint 489-516: Missing bridge hook call in UpdateOracleConfig

Unlike other update methods, UpdateOracleConfig does not invoke a bridge hook after updating the configuration. To maintain consistency and ensure that any necessary side effects are handled, consider adding a bridge hook call similar to other update methods.

If a BridgeOracleConfigUpdated hook exists, you can add:

if err := ms.Keeper.bridgeHook.BridgeOracleConfigUpdated(ctx, bridgeId, config); err != nil {
    return nil, err
}

This should be placed after updating config.OracleEnabled and before setting the updated configuration.


Line range hint 519-538: Consider emitting an event after updating parameters in UpdateParams

In UpdateParams, after successfully updating the parameters, it would be beneficial to emit an event indicating that the parameters have changed. This can aid in tracking configuration changes and debugging.

Add event emission:

sdk.UnwrapSDKContext(ctx).EventManager().EmitEvent(sdk.NewEvent(
    types.EventTypeUpdateParams,
    sdk.NewAttribute(types.AttributeKeyAuthority, req.Authority),
))

Line range hint 350-388: Potential nil pointer dereference when accessing finalizedOutput

In UpdateProposer, after calling GetLastFinalizedOutput, the code accesses finalizedOutput.L2BlockNumber without checking if finalizedOutput is nil. If GetLastFinalizedOutput can return a nil finalizedOutput, this could lead to a runtime panic.

Ensure that finalizedOutput is not nil before accessing its fields:

if finalizedOutput == nil {
    // Handle the nil case appropriately
}

Alternatively, adjust GetLastFinalizedOutput to guarantee a non-nil finalizedOutput.


Line range hint 440-487: Event attributes may contain outdated values after updating config

In UpdateBatchInfo, after updating config.BatchInfo, the event attributes use req.NewBatchInfo directly. If any modifications to config.BatchInfo occur within hooks or update functions, the event attributes might not reflect the current state. Consider using the updated config object when constructing event attributes.

Use config.BatchInfo in event attributes to ensure consistency:

sdk.UnwrapSDKContext(ctx).EventManager().EmitEvent(sdk.NewEvent(
    types.EventTypeUpdateBatchInfo,
    sdk.NewAttribute(types.AttributeKeyBridgeId, strconv.FormatUint(bridgeId, 10)),
    sdk.NewAttribute(types.AttributeKeyBatchChainType, config.BatchInfo.ChainType.StringWithoutPrefix()),
    sdk.NewAttribute(types.AttributeKeyBatchSubmitter, config.BatchInfo.Submitter),
    // ...
))
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between b1e4e81 and 6581f3f.

📒 Files selected for processing (1)
  • x/ophost/keeper/msg_server.go (2 hunks)
🧰 Additional context used

@initia-labs initia-labs deleted a comment from coderabbitai bot Oct 22, 2024
Copy link
Contributor

@beer-1 beer-1 left a comment

Choose a reason for hiding this comment

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

LGTM

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

🧹 Outside diff range and nitpick comments (2)
contrib/launchtools/README.md (1)

41-45: LGTM. Update documentation for new parameter.

The changes to the op_bridge section improve readability and add new functionality:

  • Time-based parameters now use a more human-readable format.
  • The enable_oracle parameter has been added.

These changes look good and enhance the configuration.

Please ensure that the documentation is updated to explain the purpose and impact of the new enable_oracle parameter.

contrib/launchtools/utils/address.go (1)

39-81: Add unit tests for the new address derivation functions.

The functions DeriveL1Address, DeriveL2Address, and DeriveDAAddress are critical for address derivation and should be thoroughly tested to ensure they work correctly with various mnemonics and chain types.

Would you like assistance in creating unit tests for these functions?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 6581f3f and e6ea82d.

📒 Files selected for processing (5)
  • README.md (1 hunks)
  • contrib/launchtools/README.md (2 hunks)
  • contrib/launchtools/config.go (9 hunks)
  • contrib/launchtools/steps/opbridge.go (1 hunks)
  • contrib/launchtools/utils/address.go (3 hunks)
✅ Files skipped from review due to trivial changes (1)
  • README.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • contrib/launchtools/config.go
🧰 Additional context used
🔇 Additional comments (6)
contrib/launchtools/README.md (4)

49-49: LGTM. Ensure consistent usage of l2_address.

The change from address to l2_address for the validator is correct and aligns with the L1/L2 address distinction.

As noted in a previous review, please ensure that all references to the validator's address throughout the codebase have been updated to use l2_address consistently.


Line range hint 68-81: LGTM. Genesis accounts updated consistently.

The removal of entries for output_submitter and batch_submitter from the genesis_accounts section aligns with the changes made in the system_keys section. This change maintains consistency in the configuration.


53-54: LGTM. Verify da_address usage for batch submitter.

The updates to address fields for various roles consistently apply the L1/L2 address distinction, improving clarity and specificity in the configuration. The changes look good overall.

Please verify the use of da_address for the batch submitter:

#!/bin/bash
echo "Checking batch submitter address usage:"
rg "batch.*submitter.*address" --type go

Ensure that all relevant code has been updated to use da_address instead of l1_address for the batch submitter.

Also applies to: 58-58, 61-61, 64-65


31-32: LGTM. Verify network upgrade implications.

The chain_id and rpc_url have been updated, likely as part of a network upgrade from "initiation-1" to "initiation-2". These changes look good.

Please ensure that all related components and documentation are updated to reflect this network change. Run the following script to check for any remaining references to the old network:

✅ Verification successful

Verified removal of "initiation-1" references.

All instances of "initiation-1" have been successfully removed. The remaining references to "INITIA|CELESTIA" are unrelated to the network upgrade.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
echo "Searching for old network references:"
rg "initiation-1" --type-not md

Length of output: 342


Script:

#!/bin/bash
echo "Performing a comprehensive search for any variations of 'initiation-1':"
rg -i "initiation-1" --type-not md

Length of output: 193


Script:

#!/bin/bash
echo "Searching for 'INITIA|CELESTIA' references:"
rg "INITIA\|CELESTIA" --type-not md

Length of output: 351

contrib/launchtools/steps/opbridge.go (1)

47-47: Verify the use of DAAddress for batch submission

The change from L1Address to DAAddress for the batch submitter appears to be part of a larger architectural change. This modification aligns with updates in other files, particularly the addition of the DAAddress field in the SystemAccount struct in config.go.

  1. Please confirm that using DAAddress instead of L1Address for the batch submitter is the intended behavior.
  2. Consider adding a comment explaining the rationale behind this change, which will help future maintainers understand the distinction between L1 and DA addresses in this context.
  3. Ensure that this change is consistently applied across the codebase, particularly in any other locations where the batch submitter's address is used.

To help verify the consistency of this change, you can run the following script:

This will help identify any places where the old L1Address might still be in use and confirm the new DAAddress is being used consistently.

✅ Verification successful

BatchSubmitter address update verified

All instances of BatchSubmitter.L1Address have been replaced with BatchSubmitter.DAAddress. The change is consistent across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining uses of BatchSubmitter.L1Address and compare with BatchSubmitter.DAAddress usage

echo "Occurrences of BatchSubmitter.L1Address:"
rg "BatchSubmitter\.L1Address" --type go

echo "\nOccurrences of BatchSubmitter.DAAddress:"
rg "BatchSubmitter\.DAAddress" --type go

Length of output: 434

contrib/launchtools/utils/address.go (1)

25-27: Verify the Bech32 prefix for Celestia addresses.

Ensure that "celestia" is the correct Bech32 prefix for Celestia addresses. Using an incorrect prefix can lead to invalid address generation.

@@ -1,9 +1,17 @@
package utils

import (
"github.com/pkg/errors"
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using the standard errors package instead of github.com/pkg/errors.

Starting from Go 1.13, the standard library's errors package provides error wrapping features like errors.Is, errors.As, and fmt.Errorf with %w. Using the standard package can reduce dependencies and streamline error handling.

Comment on lines +47 to +58
func DeriveDAAddress(mnemonic string, chainType ophosttypes.BatchInfo_ChainType) (string, error) {
var codec address.Codec
switch chainType {
case ophosttypes.BatchInfo_CHAIN_TYPE_INITIA:
codec = L1AddressCodec()
case ophosttypes.BatchInfo_CHAIN_TYPE_CELESTIA:
codec = CelestiaAddressCodec()
default:
return "", errors.New("unsupported chain type")
}
return deriveAddress(mnemonic, codec)
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error message for unsupported chain types.

Including the unsupported chain type in the error message can aid in debugging and provide clearer feedback.

Apply this diff to enhance the error message:

-		return "", errors.New("unsupported chain type")
+		return "", errors.Errorf("unsupported chain type: %v", chainType)
📝 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.

Suggested change
func DeriveDAAddress(mnemonic string, chainType ophosttypes.BatchInfo_ChainType) (string, error) {
var codec address.Codec
switch chainType {
case ophosttypes.BatchInfo_CHAIN_TYPE_INITIA:
codec = L1AddressCodec()
case ophosttypes.BatchInfo_CHAIN_TYPE_CELESTIA:
codec = CelestiaAddressCodec()
default:
return "", errors.New("unsupported chain type")
}
return deriveAddress(mnemonic, codec)
}
func DeriveDAAddress(mnemonic string, chainType ophosttypes.BatchInfo_ChainType) (string, error) {
var codec address.Codec
switch chainType {
case ophosttypes.BatchInfo_CHAIN_TYPE_INITIA:
codec = L1AddressCodec()
case ophosttypes.BatchInfo_CHAIN_TYPE_CELESTIA:
codec = CelestiaAddressCodec()
default:
return "", errors.Errorf("unsupported chain type: %v", chainType)
}
return deriveAddress(mnemonic, codec)
}

func deriveAddress(mnemonic string, codec address.Codec) (string, error) {
addrBz, err := deriveAddressBz(mnemonic)
if err != nil {
return "", errors.Wrap(err, "failed to convert address to bech32")
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct the error message to reflect the actual failure point.

The error message suggests a failure in converting to Bech32, but the error actually occurs during address byte derivation. Update the message for clarity.

Apply this diff to correct the error message:

-		return "", errors.Wrap(err, "failed to convert address to bech32")
+		return "", errors.Wrap(err, "failed to derive address bytes")
📝 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.

Suggested change
return "", errors.Wrap(err, "failed to convert address to bech32")
return "", errors.Wrap(err, "failed to derive address bytes")

@sh-cha sh-cha merged commit 414508a into main Oct 22, 2024
7 of 8 checks passed
@sh-cha sh-cha deleted the fix/celestia-codec branch October 22, 2024 08:00
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