feat(genesis): use embedded genesis config for default chains #56
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe recent changes primarily streamline the configuration handling for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Orchestrator
participant Anvil
participant Config
User->>Orchestrator: Request to deploy L1 & L2 chains
Orchestrator->>Config: Fetch chain configurations
Config-->>Orchestrator: Return L2Config with L1ChainID
Orchestrator->>Anvil: Initialize with ChainConfig
Anvil->>Anvil: Process genesis data as JSON
Orchestrator->>User: Confirm deployment success
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
jakim929
changed the title
jakim929
force-pushed
the
07-18-feat_genesis_embed_and_unmarshal_genesis_files
branch
4 times, most recently
from
49205cb to
ef2689a
Compare
jakim929
force-pushed
the
07-18-feat_genesis_embed_and_unmarshal_genesis_files
branch
from
ef2689a to
1dcae6c
Compare
jakim929
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
genesisdeployment/genesisdeployment.go (1)
64-74: Implement proper error handling innewL2GenesisDeploymentusageThe
newL2GenesisDeploymentfunction is used within a struct initialization without any error handling. This means any error during JSON unmarshalling will cause a panic, which is not ideal for robust error handling.
- Consider logging the error and handling it gracefully instead of using
panic.Analysis chain
Function implementation looks good. Verify error handling.
The
newL2GenesisDeploymentfunction correctly handles JSON unmarshalling and struct initialization.However, consider logging the error instead of using
panicfor better error handling.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `newL2GenesisDeployment` function for error handling. # Test: Search for the function usage. Expect: Proper error handling in calling code. rg --type go 'newL2GenesisDeployment'Length of output: 731
Script:
#!/bin/bash # Description: Extract context around the usages of `newL2GenesisDeployment` in genesisdeployment/genesisdeployment.go. # Extract 5 lines before and after each match to understand error handling. rg --type go 'newL2GenesisDeployment' -A 5 -B 5 genesisdeployment/genesisdeployment.goLength of output: 996
orchestrator/orchestrator.go (1)
Line range hint
33-71:
LGTM! Consider refactoring to reduce complexity.The changes streamline the configuration process and ensure only one L1 chain is supported. However, the function is becoming complex and could benefit from refactoring to improve readability and maintainability.
func NewOrchestrator(log log.Logger, chainConfigs []config.ChainConfig) (*Orchestrator, error) { var opSimInstances []*opsimulator.OpSimulator var anvilInstances []*anvil.Anvil l1Count := 0 for _, config := range chainConfigs { if config.L2Config == nil { l1Count++ } } if l1Count > 1 { return nil, fmt.Errorf("supersim does not support more than one l1") } // Spin up anvil instances var l1Chain *anvil.Anvil anvilInstanceByChainID := make(map[uint64]*anvil.Anvil) for _, chainConfig := range chainConfigs { anvilConfig := chainConfig if chainConfig.L2Config != nil { anvilConfig.Port = 0 // internally allocate anvil port as op-simulator port is exposed externally } // Apply genesis if not forking from a live network if chainConfig.ForkConfig == nil { anvilConfig.GenesisJSON = chainConfig.GenesisJSON } anvil := anvil.New(log, &anvilConfig) anvilInstances = append(anvilInstances, anvil) anvilInstanceByChainID[chainConfig.ChainID] = anvil if chainConfig.L2Config == nil { l1Chain = anvil } } // Create Op Simulators to front L2 chains. for _, chainConfig := range chainConfigs { if chainConfig.L2Config != nil { opSim := opsimulator.New(log, chainConfig.Port, l1Chain, anvilInstanceByChainID[chainConfig.ChainID], chainConfig.L2Config) opSimInstances = append(opSimInstances, opSim) } } return &Orchestrator{log, l1Chain, opSimInstances, anvilInstances, anvilInstanceByChainID}, nil }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (11)
- anvil/anvil.go (7 hunks)
- anvil/anvil_test.go (1 hunks)
- config/chain.go (4 hunks)
- genesisdeployment/genesisdeployment.go (1 hunks)
- go.mod (1 hunks)
- opsimulator/opsimulator.go (3 hunks)
- orchestrator/fork.go (4 hunks)
- orchestrator/orchestrator.go (4 hunks)
- orchestrator/orchestrator_test.go (2 hunks)
- supersim_test.go (1 hunks)
- testutils/mockchain.go (3 hunks)
Additional comments not posted (28)
anvil/anvil_test.go (1)
18-18: Simplify initialization ofcfgvariable.The change simplifies the initialization of the
cfgvariable by directly usingconfig.ChainConfiginstead of wrapping it in an outerConfigtype. This is a positive change that reduces unnecessary complexity.testutils/mockchain.go (2)
9-9: New import statement forcommonpackage.The import statement for
github.com/ethereum/go-ethereum/commonwas added to accommodate the new method's parameter type.
46-48: AddEthGetCodemethod toMockChain.The new method
EthGetCodeis a placeholder that returns an empty byte slice and a nil error. This method is likely added for future implementation or testing purposes.orchestrator/orchestrator_test.go (2)
24-25: Update chain configuration structure increateTestSuite.The
SourceChainIDfield has been replaced with anL2Configfield in thechainConfigsslice. This change aligns with the new logic or requirement related to Layer 2 configurations.
59-59: Update chain configuration structure inTestTooManyL1sError.The
SourceChainIDfield has been replaced with anL2Configfield in thechainConfigsslice. This change aligns with the new logic or requirement related to Layer 2 configurations.genesisdeployment/genesisdeployment.go (3)
14-15: Correct usage ofgo:embeddirective.The embedded genesis files and addresses are correctly referenced and organized.
Also applies to: 20-33, 38-51
53-62: Struct definitions look good.The
L1GenesisDeploymentandL2GenesisDeploymentstructs correctly encapsulate the required data.
77-94: Struct and variable initialization look good.The
GenesisDeploymentstruct andGeneratedGenesisDeploymentvariable correctly encapsulate the genesis data.config/chain.go (5)
8-14: Import statements look good.The imported packages are necessary and correctly referenced.
66-69: Struct definition looks good.The
L2Configstruct correctly encapsulates the required data.
75-86: Struct modifications look good.The
ChainConfigstruct modifications correctly integrate the newL2Configfield.
96-99: Interface modifications look good.The
Chaininterface modifications correctly integrate the new method and reorder the existing method.
28-50: Initialization ofDefaultChainConfigslooks good. Verify dynamic values.The initialization correctly references the dynamic values from
GeneratedGenesisDeployment.However, consider verifying that the dynamic values are correctly set and used throughout the codebase.
Verification successful
Initialization of
DefaultChainConfigsand dynamic values usage is verified.The
DefaultChainConfigsinitialization and its usage insupersim.goare correct. The functionChainConfigsFromForkCLIConfigproperly processes theforkConfigand initializes the chain configurations dynamically.
supersim.go: Properly usesDefaultChainConfigsand overrides it ifcliConfig.ForkConfigis provided.orchestrator/fork.go: Correctly processesforkConfigto initialize dynamic chain configurations.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of dynamic values in `DefaultChainConfigs`. # Test: Search for the usage of `DefaultChainConfigs`. Expect: Proper usage of dynamic values. rg --type go 'DefaultChainConfigs'Length of output: 144
Script:
#!/bin/bash # Description: Extract the context around the usage of `DefaultChainConfigs` in `supersim.go`. # Get the lines around the usage of `DefaultChainConfigs` in `supersim.go`. rg --type go --context 10 'DefaultChainConfigs' supersim.goLength of output: 707
Script:
#!/bin/bash # Description: Extract the implementation of `orchestrator.ChainConfigsFromForkCLIConfig`. # Search for the implementation of `ChainConfigsFromForkCLIConfig` in the orchestrator package. rg --type go 'func ChainConfigsFromForkCLIConfig' --context 10Length of output: 1167
Script:
#!/bin/bash # Description: Extract the complete implementation of `ChainConfigsFromForkCLIConfig` from `orchestrator/fork.go`. # Get the complete implementation of `ChainConfigsFromForkCLIConfig`. rg --type go --context 50 'func ChainConfigsFromForkCLIConfig' orchestrator/fork.goLength of output: 2188
opsimulator/opsimulator.go (4)
Line range hint
8-14:
Import statements look good.The imported packages are necessary and correctly referenced.
33-34: Struct modifications look good.The
OpSimulatorstruct modifications correctly integrate the newL2Configfield.
48-54: Constructor function modifications look good.The
Newconstructor function modifications correctly accept the newL2Configparameter and initialize the struct.
161-161: Method modifications look good.The
SourceChainIDmethod modifications correctly return theL1ChainIDfrom theL2Config.orchestrator/orchestrator.go (2)
20-21: LGTM!The addition of the
l1Anvilfield to theOrchestratorstruct is clear and aligns with the intended functionality.
170-171: LGTM!The simplification of the
L1Anvilfunction improves performance and clarity.go.mod (1)
39-39: LGTM!The addition of the
github.com/ethereum-optimism/go-ethereum-hdwalletdependency is clear and aligns with the intended functionality.orchestrator/fork.go (3)
52-52: Ensure theaddressListretrieval is correct.The retrieval of
addressListfrom the registry looks correct, but verify thatconfig.OpChainToId[chain]always returns a valid key.
68-71: LGTM! TheL2Configintegration looks good.The integration of
L2ConfigwithL1ChainIDandL1DeploymentAddressesis correct and follows the intended design.
113-136: LGTM! The mapping inaddressListToL1Deploymentsis correct.The mapping from
AddressListtoL1Deploymentsis well-implemented and follows the intended design. The comments clearly indicate the temporary nature of the function.anvil/anvil.go (5)
Line range hint
51-58:
LGTM! The changes in theNewmethod are consistent.The update to accept
*config.ChainConfiginstead of*Configis correct and consistent with the new design.
76-81: Ensure proper handling of the genesis file.The logic for handling
GenesisJSONand writing it to a temporary file is correct. Ensure that the temporary file is properly managed and deleted after use.
265-267: LGTM! TheEthGetCodemethod is correctly implemented.The method correctly retrieves the code at a specified Ethereum address using the
ethClient.
268-270: LGTM! TheEthGetLogsmethod is correctly implemented.The method correctly retrieves logs using the
ethClient.
Line range hint
272-274:
LGTM! TheSubscribeFilterLogsmethod is correctly implemented.The method correctly subscribes to logs using the
ethClient.
Summary by CodeRabbit
New Features
Anvil, removing the intermediateConfigtype.EthGetCodemethod.Bug Fixes
Orchestrator.Tests
Chores
github.com/ethereum-optimism/go-ethereum-hdwallet.