Contract addresses: Have host fetch L2 Msg Bus address from enclave #1669
Conversation
WalkthroughThe changes across various Go files involve the introduction of functionality to handle the L2 message bus address within an enclave context. New functions and methods have been added to interfaces, structs, and RPC-related code to retrieve and verify the L2 message bus address. These additions are part of a broader update to include L2 message bus address information in network configurations and to ensure its availability through RPC calls and network tests. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- go/common/rpc/generated/enclave.pb.go
- go/common/rpc/generated/enclave.proto
- go/common/rpc/generated/enclave_grpc.pb.go
Files selected for processing (9)
- go/common/enclave.go (1 hunks)
- go/common/query_types.go (1 hunks)
- go/enclave/crosschain/processors.go (2 hunks)
- go/enclave/enclave.go (1 hunks)
- go/enclave/rpc_server.go (1 hunks)
- go/host/host.go (1 hunks)
- go/host/rpc/enclaverpc/enclave_client.go (1 hunks)
- integration/networktest/actions/l1/important_contracts.go (2 hunks)
- integration/networktest/tests/bridge/important_contracts_test.go (1 hunks)
Additional comments: 7
go/common/query_types.go (1)
- 89-94: The addition of
L2MessageBusAddressto theObscuroNetworkInfostruct is consistent with the surrounding code and does not require JSON tags or comments based on the existing structure. Ensure that all parts of the codebase that instantiate or useObscuroNetworkInfoare updated to handle the new field appropriately.go/enclave/crosschain/processors.go (1)
- 36-38: The method
GetL2MessageBusAddressassumes thatGetBusAddresswill never return nil. If there's a possibility forGetBusAddressto return nil, this could lead to a nil pointer dereference. Please ensure thatGetBusAddresshas proper nil checks or document that it will always return a non-nil value.go/enclave/enclave.go (1)
- 1381-1383: Ensure that the
crossChainProcessorsfield is properly initialized before callingGetL2MessageBusAddressto prevent potential null reference exceptions.go/enclave/rpc_server.go (1)
- 439-439: The function
GetL2MessageBusAddresscorrectly returns the address in byte format, which is consistent with the expected response format for addresses in the system.go/host/host.go (1)
- 202-215: The changes to
ObscuroConfigto include the L2 message bus address are correctly implemented and match the summary provided.integration/networktest/actions/l1/important_contracts.go (1)
- 6-6: The addition of the
actionspackage import is appropriate for the new functionVerifyL2MessageBusAddressAvailable.integration/networktest/tests/bridge/important_contracts_test.go (1)
- 17-26: The changes to the
TestImportantContractsLookupfunction correctly introduce the intended tests for verifying the setting and retrieval of an important contract's address and the availability of the L2 Message Bus address. Ensure that the hardcoded address0x64is appropriate for the test environment.
go/common/enclave.go
Outdated
| // GetL2MessageBusAddress returns the address of the L2 message bus contract | ||
| GetL2MessageBusAddress() (gethcommon.Address, SystemError) |
There was a problem hiding this comment.
The new function GetL2MessageBusAddress should have a comment explaining its purpose, parameters, and return values, as it is an exported function and part of an interface.
| func (c *Client) GetL2MessageBusAddress() (gethcommon.Address, common.SystemError) { | ||
| timeoutCtx, cancel := context.WithTimeout(context.Background(), c.config.EnclaveRPCTimeout) | ||
| defer cancel() | ||
|
|
||
| response, err := c.protoClient.GetL2MessageBusAddress(timeoutCtx, &generated.GetL2MessageBusAddressRequest{}) | ||
| if err != nil { | ||
| return gethcommon.Address{}, syserr.NewRPCError(err) | ||
| } | ||
| if response != nil && response.SystemError != nil { | ||
| return gethcommon.Address{}, syserr.NewInternalError(fmt.Errorf("%s", response.SystemError.ErrorString)) | ||
| } | ||
|
|
||
| return gethcommon.BytesToAddress(response.L2MessageBusAddress), nil | ||
| } |
There was a problem hiding this comment.
The function GetL2MessageBusAddress is implemented correctly and follows the established patterns in the codebase. However, it lacks documentation. It is recommended to add a comment explaining the purpose of the function, its return values, and any potential side effects or error conditions.
| func VerifyL2MessageBusAddressAvailable() networktest.Action { | ||
| // VerifyOnly because the L2MessageBus should be deployed automatically by the node as a synthetic tx | ||
| return actions.VerifyOnlyAction(func(ctx context.Context, network networktest.NetworkConnector) error { | ||
| cli, err := obsclient.Dial(network.ValidatorRPCAddress(0)) |
There was a problem hiding this comment.
Consider making the validator index configurable instead of hardcoding it to 0 to improve flexibility in case of multiple validators or changes in network configuration.
| return errors.New("L2MessageBusAddress not set") | ||
|
|
||
| } | ||
| fmt.Println("L2MessageBusAddress: ", networkCfg.L2MessageBusAddress) |
There was a problem hiding this comment.
Replace fmt.Println with a structured logging approach to ensure that the output can be properly managed and observed in production environments.
go/enclave/rpc_server.go
Outdated
| func (s *RPCServer) GetL2MessageBusAddress(_ context.Context, _ *generated.GetL2MessageBusAddressRequest) (*generated.GetL2MessageBusAddressResponse, error) { | ||
| address, sysError := s.enclave.GetL2MessageBusAddress() | ||
| if sysError != nil { | ||
| s.logger.Error("Error getting message bus address", log.ErrKey, sysError) | ||
| return &generated.GetL2MessageBusAddressResponse{SystemError: toRPCError(sysError)}, nil | ||
| } | ||
| return &generated.GetL2MessageBusAddressResponse{L2MessageBusAddress: address.Bytes()}, nil |
There was a problem hiding this comment.
The new function GetL2MessageBusAddress lacks documentation. It's best practice to provide comments explaining the purpose and usage of exported functions.
go/enclave/enclave.go
Outdated
| func (e *enclaveImpl) GetL2MessageBusAddress() (gethcommon.Address, common.SystemError) { | ||
| return e.crossChainProcessors.GetL2MessageBusAddress() | ||
| } |
There was a problem hiding this comment.
The new method GetL2MessageBusAddress is added without documentation. Consider adding a comment explaining the purpose and usage of this method.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- go/enclave/crosschain/processors.go (2 hunks)
- integration/networktest/actions/l1/important_contracts.go (2 hunks)
Additional comments: 4
go/enclave/crosschain/processors.go (2)
6-6: The import of the package "github.com/ten-protocol/go-ten/go/common" is correctly added to support the new function's return type.
37-39: The implementation of
GetL2MessageBusAddressappears correct, assuming thatGetBusAddressfromLocalhandles nil cases properly.integration/networktest/actions/l1/important_contracts.go (2)
114-114: Consider making the validator index configurable instead of hardcoding it to
0to improve flexibility in case of multiple validators or changes in network configuration.127-127: Replace
fmt.Printlnwith a structured logging approach to ensure that the output can be properly managed and observed in production environments.
go/host/host.go
Outdated
| l2MessageBusAddress, err := h.EnclaveClient().GetL2MessageBusAddress() | ||
| if err != nil { | ||
| return nil, responses.ToInternalError(fmt.Errorf("unable to get L2 message bus address - %w", err)) | ||
| } |
There was a problem hiding this comment.
Should we have a generic enclave config method instead of the specific GetL2MessageBusAddress ? Perhaps I'm future-proofing..
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- go/common/rpc/generated/enclave.pb.go
- go/common/rpc/generated/enclave.proto
- go/common/rpc/generated/enclave_grpc.pb.go
Files selected for processing (5)
- go/common/enclave.go (2 hunks)
- go/enclave/enclave.go (1 hunks)
- go/enclave/rpc_server.go (1 hunks)
- go/host/host.go (1 hunks)
- go/host/rpc/enclaverpc/enclave_client.go (1 hunks)
Additional comments: 7
go/common/enclave.go (2)
138-142: The addition of
GetPublicTransactionDataandEnclavePublicConfigto theEnclaveScaninterface aligns with the summary and seems to be correctly implemented.158-160: The
EnclavePublicConfigstruct is correctly defined with theL2MessageBusAddressfield of typegethcommon.Address.go/enclave/enclave.go (1)
- 1381-1387: The implementation of
EnclavePublicConfigmethod is correct and follows best practices for error handling and struct creation.go/enclave/rpc_server.go (1)
- 439-439: The implementation of
EnclavePublicConfigcorrectly returns the L2 message bus address in byte format, which is consistent with how Ethereum addresses are typically handled in the system.go/host/host.go (2)
202-215: The changes to the
ObscuroConfigfunction correctly implement the retrieval of the L2 message bus address from the enclave and its inclusion in theObscuroNetworkInfostruct. This aligns with the pull request's description.213-213: Verify that the
L2MessageBusAddressobtained from the enclave is validated before use in other parts of the codebase to ensure it meets any expected format or security requirements.go/host/rpc/enclaverpc/enclave_client.go (1)
- 602-615: The implementation of the
EnclavePublicConfigfunction correctly handles timeouts and errors, and properly constructs theEnclavePublicConfigstruct from the RPC response. The code is consistent with the rest of the codebase and follows best practices for error handling and context usage.
| func (s *RPCServer) EnclavePublicConfig(_ context.Context, _ *generated.EnclavePublicConfigRequest) (*generated.EnclavePublicConfigResponse, error) { | ||
| enclaveCfg, sysError := s.enclave.EnclavePublicConfig() | ||
| if sysError != nil { | ||
| s.logger.Error("Error getting message bus address", log.ErrKey, sysError) | ||
| return &generated.EnclavePublicConfigResponse{SystemError: toRPCError(sysError)}, nil | ||
| } | ||
| return &generated.EnclavePublicConfigResponse{L2MessageBusAddress: enclaveCfg.L2MessageBusAddress.Bytes()}, nil | ||
| } |
There was a problem hiding this comment.
The new function EnclavePublicConfig lacks documentation. It's important to maintain consistency and provide clear documentation for all public functions to explain their purpose, parameters, return values, and any side effects or errors they might produce.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- go/host/host.go (3 hunks)
Additional comments: 2
go/host/host.go (2)
7-7: The import of
github.com/ethereum/go-ethereum/commonis correctly added to support the newl2MessageBusAddressfield.48-49: The new field
l2MessageBusAddressis correctly added to thehoststruct to cache the L2 message bus address.
| if h.l2MessageBusAddress == nil { | ||
| publicCfg, err := h.EnclaveClient().EnclavePublicConfig() | ||
| if err != nil { | ||
| return nil, responses.ToInternalError(fmt.Errorf("unable to get L2 message bus address - %w", err)) | ||
| } | ||
| h.l2MessageBusAddress = &publicCfg.L2MessageBusAddress |
There was a problem hiding this comment.
The potential data race issue when accessing l2MessageBusAddress still exists. Consider adding synchronization, such as a mutex, to prevent multiple goroutines from making redundant calls to EnclaveClient().EnclavePublicConfig().
Why this change is needed
Unlike other network contract deployments the L2 message bus is deployed by the enclave with a sythetic tx.
Until we have a better method available this change has the host fetching that address from the enclave so it can serve it with the other contract addresses.
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks