Harmony rpc #1735
Harmony rpc #1735
Conversation
WalkthroughThis update enriches the Changes
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (9)
Files not reviewed due to errors (1)
Additional comments not posted (8)
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 Configration File (
|
![coderabbitai[bot]](https://github.com/avatars/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 selected for processing (10)
- ethergo/client/capture_client.go (3 hunks)
- ethergo/client/client.go (3 hunks)
- ethergo/client/hmy.go (1 hunks)
- ethergo/client/methods.go (1 hunks)
- ethergo/client/mocks/evm.go (1 hunks)
- services/omnirpc/cmd/cmd.go (1 hunks)
- services/omnirpc/cmd/commands.go (1 hunks)
- services/omnirpc/cmd/flags.go (1 hunks)
- services/omnirpc/metadata/buildinfo.go (1 hunks)
- services/omnirpc/proxy/simpleproxy.go (1 hunks)
Additional comments: 13
services/omnirpc/cmd/flags.go (1)
- 44-47: The addition of
rpcFlagis consistent with the existing pattern of flag declarations and is necessary for specifying the RPC URL.services/omnirpc/cmd/cmd.go (1)
- 25-25: The addition of the
latestRewritecommand to theapp.Commandsslice should be verified for correct integration with the existing command-line interface.ethergo/client/capture_client.go (1)
- 18-18: The addition of the
rpcClientfield to thecaptureClientstruct is consistent with the existing structure pattern and is properly initialized in thenewCaptureClientfunction.ethergo/client/hmy.go (1)
- 14-20: The
HarmonyVMinterface is a well-defined extension of theEVMinterface, providing Harmony-specific methods. Ensure that these new methods are consistently used throughout the application.Verification successful
The search results indicate that the
HarmonyVMinterface and its methodsFilterHarmonyLogsandHarmonyTransactionReceiptare defined inethergo/client/hmy.goand are used inservices/omnirpc/proxy/simpleproxy.go. This suggests that the new Harmony-specific methods are indeed being utilized within the application.
- The
HarmonyVMinterface is defined and includes the methodsFilterHarmonyLogsandHarmonyTransactionReceipt.- The
harmonyClientstruct implements these methods.- The
simpleproxy.gofile uses theHarmonyTransactionReceiptmethod, indicating integration of the new functionality into the proxy service.Based on the information provided, it appears that the new Harmony-specific methods are being used consistently within the application.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new Harmony-specific methods are used consistently. rg --type go "HarmonyVM|FilterHarmonyLogs|HarmonyTransactionReceipt"Length of output: 1315
ethergo/client/methods.go (2)
63-66: The new Harmony RPC methods
HarmonyGetLogsMethodandHarmonyGetReceiptMethodare correctly added and follow the existing pattern of RPC method declarations.72-72: The modification of
NetVersionMethodappears redundant as the value remains unchanged. Please confirm if this change was intentional.services/omnirpc/cmd/commands.go (1)
- 158-174: The
latestRewritecommand is correctly added with appropriate flags for RPC and port configurations, and the action to run a simple proxy is well-defined.services/omnirpc/proxy/simpleproxy.go (2)
56-88: The
Runmethod sets up the HTTP server and routes. It is important to ensure that the server is only accessible within a secure and controlled network environment, as binding to0.0.0.0allows connections from any IP address.158-207: The
verifyHarmonyRequestmethod handles Harmony-specific requests. It is important to ensure that the Harmony node being proxied to is trusted, as the proxy forwards requests and potentially sensitive data.ethergo/client/client.go (3)
55-56: The addition of the
Web3Versionmethod to theEVMinterface and its implementation inclientImplis consistent with the PR's objective to extend blockchain support.85-85: The assignment of the
rpcClientfield in theDialBackendfunction is correctly done using thecaptureClientcreated within the same function.263-277: The implementation of the
Web3Versionmethod in theclientImplstruct is correct, with proper error handling and tracing.ethergo/client/mocks/evm.go (1)
- 779-798: The addition of the
Web3Versionmock function is correctly implemented and follows the standard mock function pattern used in this file.
| @@ -10,5 +10,5 @@ var ( | |||
|
|
|||
| // BuildInfo returns the build info for the service. | |||
| func BuildInfo() config.BuildInfo { | |||
| return config.NewBuildInfo(version, commit, "omnirpc", date) | |||
| return config.NewBuildInfo(version, commit, "omnirpcharmony", date) | |||
There was a problem hiding this comment.
The search for the string "omnirpcharmony" within Go files only returned a single occurrence, which is in the services/omnirpc/metadata/buildinfo.go file. This suggests that the new service name "omnirpcharmony" is not used elsewhere in the application, at least not in the Go source files.
- The
BuildInfofunction inservices/omnirpc/metadata/buildinfo.gohas been updated to use "omnirpcharmony" as the service name. - The string "omnirpcharmony" does not appear in any other Go files in the application.
Given this information, it is not possible to confirm that the new service name is consistent with the rest of the application, as it appears to be introduced here and not used elsewhere.
Analysis chain
The change in the service name within the BuildInfo function to "omnirpcharmony" should be consistent with the service's naming conventions and usage throughout the application.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the new service name is used consistently across the application.
rg --type go "omnirpcharmony"
Length of output: 137
| g.Go(func() error { | ||
| harmonyReceipt, err = hmyClient.HarmonyTransactionReceipt(gCtx, txHash) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get harmony receipt: %w", err) | ||
| } | ||
| return nil | ||
| }) | ||
|
|
||
| var rpcMessage JSONRPCMessage | ||
|
|
||
| g.Go(func() error { | ||
| /// no need to double up on this check when doing receipts | ||
| if checkVersion { | ||
| web3Version, err := hmyClient.Web3Version(gCtx) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get web3 version: %w", err) | ||
| } | ||
|
|
||
| if !strings.Contains(web3Version, expectedVersion) { | ||
| return fmt.Errorf("expected version %s, got %s", expectedVersion, web3Version) | ||
| } | ||
| } | ||
| return nil | ||
| }) | ||
|
|
||
| g.Go(func() error { | ||
| rawResp, err = r.makeReq(ctx, rawBody) | ||
| if err != nil { | ||
| return fmt.Errorf("could not make req: %w", err) | ||
| } | ||
|
|
||
| err = json.Unmarshal(rawResp, &rpcMessage) | ||
| if err != nil { | ||
| return fmt.Errorf("could not unmarshal: %w", err) | ||
| } | ||
|
|
||
| err = json.Unmarshal(rpcMessage.Result, ðReceipt) | ||
| if err != nil { | ||
| return fmt.Errorf("could not unmarshal eth receipt: %w", err) | ||
| } | ||
| return nil | ||
| }) | ||
|
|
||
| err = g.Wait() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get receipts: %w", err) | ||
| } | ||
|
|
||
| if harmonyReceipt.BlockHash != ethReceipt.BlockHash { | ||
| return nil, fmt.Errorf("expected block hash %s, got %s", harmonyReceipt.BlockHash, ethReceipt.BlockHash) | ||
| } | ||
|
|
||
| if harmonyReceipt.TxHash == ethReceipt.TxHash { | ||
| return nil, fmt.Errorf("expected different tx hashes %s, got %s", harmonyReceipt.TxHash, ethReceipt.TxHash) | ||
| } | ||
|
|
||
| if harmonyReceipt.Status != ethReceipt.Status { | ||
| return nil, fmt.Errorf("expected tx index %d, got %d", harmonyReceipt.Status, ethReceipt.Status) | ||
| } | ||
|
|
||
| if harmonyReceipt.CumulativeGasUsed != ethReceipt.CumulativeGasUsed { | ||
| return nil, fmt.Errorf("expected index %d, got %d", harmonyReceipt.CumulativeGasUsed, ethReceipt.CumulativeGasUsed) | ||
| } | ||
|
|
||
| if harmonyReceipt.GasUsed != ethReceipt.GasUsed { | ||
| return nil, fmt.Errorf("expected index %d, got %d", harmonyReceipt.GasUsed, ethReceipt.GasUsed) | ||
| } | ||
|
|
||
| if len(harmonyReceipt.Logs) != len(ethReceipt.Logs) { | ||
| return nil, fmt.Errorf("expected %d logs, got %d", len(harmonyReceipt.Logs), len(ethReceipt.Logs)) | ||
| } | ||
|
|
||
| for i := 0; i < len(harmonyReceipt.Logs); i++ { | ||
| ethReceipt.Logs[i].TxHash = ethReceipt.TxHash | ||
| } | ||
|
|
||
| receiptLogsMarshall, err := json.Marshal(ethReceipt.Logs) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not marshal eth receipt: %w", err) | ||
| } | ||
|
|
||
| var fields map[string]json.RawMessage | ||
| err = json.Unmarshal(rpcMessage.Result, &fields) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not unmarshal fields: %w", err) | ||
| } | ||
|
|
||
| fields["logs"] = json.RawMessage(receiptLogsMarshall) | ||
| rpcMessage.Result, err = json.Marshal(fields) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not marshal fields: %w", err) | ||
| } | ||
|
|
||
| rawResp, err = json.Marshal(rpcMessage) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not marshal rpc message: %w", err) | ||
| } | ||
|
|
||
| return rawResp, nil | ||
| } |
There was a problem hiding this comment.
The getHarmonyReceiptVerify method performs parallel requests to verify Harmony transaction receipts. It is crucial to handle potential data races when accessing shared resources in goroutines, such as err in line 255. Use a local variable for error handling within each goroutine to avoid data races.
- harmonyReceipt, err = hmyClient.HarmonyTransactionReceipt(gCtx, txHash)
+ harmonyReceipt, goroutineErr := hmyClient.HarmonyTransactionReceipt(gCtx, txHash)
+ if goroutineErr != nil {
+ return fmt.Errorf("could not get harmony receipt: %w", goroutineErr)
+ }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.
| func (r *SimpleProxy) getHarmonyReceiptVerify(parentCtx context.Context, txHash common.Hash, rawBody []byte, checkVersion bool) (_ []byte, err error) { | |
| ctx, span := r.tracer.Start(parentCtx, "getHarmonyReceiptVerify") | |
| defer func() { | |
| metrics.EndSpanWithErr(span, err) | |
| }() | |
| hmyClient, err := client.DialHarmonyBackend(ctx, r.proxyURL, r.handler, client.Capture(true)) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not dial harmony backend: %w", err) | |
| } | |
| var harmonyReceipt, ethReceipt *types.Receipt | |
| var rawResp []byte | |
| g, gCtx := errgroup.WithContext(ctx) | |
| g.Go(func() error { | |
| harmonyReceipt, err = hmyClient.HarmonyTransactionReceipt(gCtx, txHash) | |
| if err != nil { | |
| return fmt.Errorf("could not get harmony receipt: %w", err) | |
| } | |
| return nil | |
| }) | |
| var rpcMessage JSONRPCMessage | |
| g.Go(func() error { | |
| /// no need to double up on this check when doing receipts | |
| if checkVersion { | |
| web3Version, err := hmyClient.Web3Version(gCtx) | |
| if err != nil { | |
| return fmt.Errorf("could not get web3 version: %w", err) | |
| } | |
| if !strings.Contains(web3Version, expectedVersion) { | |
| return fmt.Errorf("expected version %s, got %s", expectedVersion, web3Version) | |
| } | |
| } | |
| return nil | |
| }) | |
| g.Go(func() error { | |
| rawResp, err = r.makeReq(ctx, rawBody) | |
| if err != nil { | |
| return fmt.Errorf("could not make req: %w", err) | |
| } | |
| err = json.Unmarshal(rawResp, &rpcMessage) | |
| if err != nil { | |
| return fmt.Errorf("could not unmarshal: %w", err) | |
| } | |
| err = json.Unmarshal(rpcMessage.Result, ðReceipt) | |
| if err != nil { | |
| return fmt.Errorf("could not unmarshal eth receipt: %w", err) | |
| } | |
| return nil | |
| }) | |
| err = g.Wait() | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get receipts: %w", err) | |
| } | |
| if harmonyReceipt.BlockHash != ethReceipt.BlockHash { | |
| return nil, fmt.Errorf("expected block hash %s, got %s", harmonyReceipt.BlockHash, ethReceipt.BlockHash) | |
| } | |
| if harmonyReceipt.TxHash == ethReceipt.TxHash { | |
| return nil, fmt.Errorf("expected different tx hashes %s, got %s", harmonyReceipt.TxHash, ethReceipt.TxHash) | |
| } | |
| if harmonyReceipt.Status != ethReceipt.Status { | |
| return nil, fmt.Errorf("expected tx index %d, got %d", harmonyReceipt.Status, ethReceipt.Status) | |
| } | |
| if harmonyReceipt.CumulativeGasUsed != ethReceipt.CumulativeGasUsed { | |
| return nil, fmt.Errorf("expected index %d, got %d", harmonyReceipt.CumulativeGasUsed, ethReceipt.CumulativeGasUsed) | |
| } | |
| if harmonyReceipt.GasUsed != ethReceipt.GasUsed { | |
| return nil, fmt.Errorf("expected index %d, got %d", harmonyReceipt.GasUsed, ethReceipt.GasUsed) | |
| } | |
| if len(harmonyReceipt.Logs) != len(ethReceipt.Logs) { | |
| return nil, fmt.Errorf("expected %d logs, got %d", len(harmonyReceipt.Logs), len(ethReceipt.Logs)) | |
| } | |
| for i := 0; i < len(harmonyReceipt.Logs); i++ { | |
| ethReceipt.Logs[i].TxHash = ethReceipt.TxHash | |
| } | |
| receiptLogsMarshall, err := json.Marshal(ethReceipt.Logs) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not marshal eth receipt: %w", err) | |
| } | |
| var fields map[string]json.RawMessage | |
| err = json.Unmarshal(rpcMessage.Result, &fields) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not unmarshal fields: %w", err) | |
| } | |
| fields["logs"] = json.RawMessage(receiptLogsMarshall) | |
| rpcMessage.Result, err = json.Marshal(fields) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not marshal fields: %w", err) | |
| } | |
| rawResp, err = json.Marshal(rpcMessage) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not marshal rpc message: %w", err) | |
| } | |
| return rawResp, nil | |
| } | |
| g.Go(func() error { | |
| harmonyReceipt, goroutineErr := hmyClient.HarmonyTransactionReceipt(gCtx, txHash) | |
| if goroutineErr != nil { | |
| return fmt.Errorf("could not get harmony receipt: %w", goroutineErr) | |
| } | |
| return nil | |
| }) |
| return nil, fmt.Errorf("could not dial harmony backend: %w", err) | ||
| } | ||
|
|
||
| var ethLogs []types.Log | ||
| var rpcMessage JSONRPCMessage | ||
|
|
||
| g, gCtx := errgroup.WithContext(ctx) | ||
| g.Go(func() error { | ||
| web3Version, err := hmyClient.Web3Version(gCtx) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get web3 version: %w", err) | ||
| } | ||
|
|
||
| if !strings.Contains(web3Version, expectedVersion) { | ||
| return fmt.Errorf("expected version %s, got %s", expectedVersion, web3Version) | ||
| } | ||
| return nil | ||
| }) | ||
|
|
||
| g.Go(func() error { | ||
| rawResp, err = r.makeReq(ctx, rawBody) | ||
| if err != nil { | ||
| return fmt.Errorf("could not make req: %w", err) | ||
| } | ||
|
|
||
| err = json.Unmarshal(rawResp, &rpcMessage) | ||
| if err != nil { | ||
| return fmt.Errorf("could not unmarshal: %w", err) | ||
| } | ||
|
|
||
| err = json.Unmarshal(rpcMessage.Result, ðLogs) | ||
| if err != nil { | ||
| return fmt.Errorf("could not unmarshal eth receipt: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
|
|
||
| err = g.Wait() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get logs: %w", err) | ||
| } | ||
|
|
||
| uniqueHashes := sets.NewString() | ||
| for i := 0; i < len(ethLogs); i++ { | ||
| uniqueHashes.Insert(ethLogs[i].TxHash.String()) | ||
| } | ||
|
|
||
| g, gCtx = errgroup.WithContext(ctx) | ||
| var logs []*types.Log | ||
| var mux sync.Mutex | ||
| for _, hash := range uniqueHashes.List() { | ||
| g.Go(func() error { | ||
| rawReqBody, err := json.Marshal(rpc.Request{ | ||
| ID: 1, | ||
| Method: client.TransactionReceiptByHashMethod.String(), | ||
| Params: []json.RawMessage{json.RawMessage(fmt.Sprintf("\"%s\"", hash))}, | ||
| }) | ||
|
|
||
| resp, err := r.getHarmonyReceiptVerify(gCtx, common.HexToHash(hash), rawReqBody, false) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get harmony receipt: %w", err) | ||
| } | ||
|
|
||
| var rpcMessage JSONRPCMessage | ||
| err = json.Unmarshal(resp, &rpcMessage) | ||
| if err != nil { | ||
| return fmt.Errorf("could not unmarshal: %w", err) | ||
| } | ||
|
|
||
| var receipt types.Receipt | ||
| err = json.Unmarshal(rpcMessage.Result, &receipt) | ||
| if err != nil { | ||
| return fmt.Errorf("could not unmarshal: %w", err) | ||
| } | ||
|
|
||
| mux.Lock() | ||
| logs = append(logs, receipt.Logs...) | ||
| mux.Unlock() | ||
| return nil | ||
| }) | ||
| } | ||
|
|
||
| err = g.Wait() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get logs: %w", err) | ||
| } | ||
|
|
||
| filteredLogs := filterLogs(logs, query.FromBlock, query.ToBlock, query.Addresses, query.Topics) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not filter logs: %w", err) | ||
| } | ||
|
|
||
| rpcMessage.Result, err = json.Marshal(filteredLogs) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not marshal fields: %w", err) | ||
| } | ||
|
|
||
| return rawResp, nil | ||
| } |
There was a problem hiding this comment.
The getLogsHarmonyVerify method contains a potential error handling issue. The error check on line 453 is redundant and will never be true because err is not reassigned after its declaration. This check should be removed or corrected to check the actual error returned from the filterLogs function.
- if err != nil {
+ // Assuming filterLogs returns an error, which is not shown in the current context.
+ filteredLogs, filterErr := filterLogs(logs, query.FromBlock, query.ToBlock, query.Addresses, query.Topics)
+ if filterErr != nil {
return nil, fmt.Errorf("could not filter logs: %w", err)
}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.
| func (r *SimpleProxy) getLogsHarmonyVerify(parentCtx context.Context, query ethereum.FilterQuery, rawBody []byte) (rawResp []byte, err error) { | |
| ctx, span := r.tracer.Start(parentCtx, "getLogsHarmonyVerify") | |
| defer func() { | |
| metrics.EndSpanWithErr(span, err) | |
| }() | |
| hmyClient, err := client.DialHarmonyBackend(ctx, r.proxyURL, r.handler, client.Capture(true)) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not dial harmony backend: %w", err) | |
| } | |
| var ethLogs []types.Log | |
| var rpcMessage JSONRPCMessage | |
| g, gCtx := errgroup.WithContext(ctx) | |
| g.Go(func() error { | |
| web3Version, err := hmyClient.Web3Version(gCtx) | |
| if err != nil { | |
| return fmt.Errorf("could not get web3 version: %w", err) | |
| } | |
| if !strings.Contains(web3Version, expectedVersion) { | |
| return fmt.Errorf("expected version %s, got %s", expectedVersion, web3Version) | |
| } | |
| return nil | |
| }) | |
| g.Go(func() error { | |
| rawResp, err = r.makeReq(ctx, rawBody) | |
| if err != nil { | |
| return fmt.Errorf("could not make req: %w", err) | |
| } | |
| err = json.Unmarshal(rawResp, &rpcMessage) | |
| if err != nil { | |
| return fmt.Errorf("could not unmarshal: %w", err) | |
| } | |
| err = json.Unmarshal(rpcMessage.Result, ðLogs) | |
| if err != nil { | |
| return fmt.Errorf("could not unmarshal eth receipt: %w", err) | |
| } | |
| return nil | |
| }) | |
| err = g.Wait() | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get logs: %w", err) | |
| } | |
| uniqueHashes := sets.NewString() | |
| for i := 0; i < len(ethLogs); i++ { | |
| uniqueHashes.Insert(ethLogs[i].TxHash.String()) | |
| } | |
| g, gCtx = errgroup.WithContext(ctx) | |
| var logs []*types.Log | |
| var mux sync.Mutex | |
| for _, hash := range uniqueHashes.List() { | |
| g.Go(func() error { | |
| rawReqBody, err := json.Marshal(rpc.Request{ | |
| ID: 1, | |
| Method: client.TransactionReceiptByHashMethod.String(), | |
| Params: []json.RawMessage{json.RawMessage(fmt.Sprintf("\"%s\"", hash))}, | |
| }) | |
| resp, err := r.getHarmonyReceiptVerify(gCtx, common.HexToHash(hash), rawReqBody, false) | |
| if err != nil { | |
| return fmt.Errorf("could not get harmony receipt: %w", err) | |
| } | |
| var rpcMessage JSONRPCMessage | |
| err = json.Unmarshal(resp, &rpcMessage) | |
| if err != nil { | |
| return fmt.Errorf("could not unmarshal: %w", err) | |
| } | |
| var receipt types.Receipt | |
| err = json.Unmarshal(rpcMessage.Result, &receipt) | |
| if err != nil { | |
| return fmt.Errorf("could not unmarshal: %w", err) | |
| } | |
| mux.Lock() | |
| logs = append(logs, receipt.Logs...) | |
| mux.Unlock() | |
| return nil | |
| }) | |
| } | |
| err = g.Wait() | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get logs: %w", err) | |
| } | |
| filteredLogs := filterLogs(logs, query.FromBlock, query.ToBlock, query.Addresses, query.Topics) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not filter logs: %w", err) | |
| } | |
| rpcMessage.Result, err = json.Marshal(filteredLogs) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not marshal fields: %w", err) | |
| } | |
| return rawResp, nil | |
| } | |
| filteredLogs, filterErr := filterLogs(logs, query.FromBlock, query.ToBlock, query.Addresses, query.Topics) | |
| if filterErr != nil { | |
| return nil, fmt.Errorf("could not filter logs: %w", filterErr) | |
| } |
| // filterLogs creates a slice of logs matching the given criteria. | ||
| func filterLogs(logs []*types.Log, fromBlock, toBlock *big.Int, addresses []common.Address, topics [][]common.Hash) []*types.Log { | ||
| var ret []*types.Log | ||
| Logs: | ||
| for _, log := range logs { | ||
| if fromBlock != nil && fromBlock.Int64() >= 0 && fromBlock.Uint64() > log.BlockNumber { | ||
| continue | ||
| } | ||
| if toBlock != nil && toBlock.Int64() >= 0 && toBlock.Uint64() < log.BlockNumber { | ||
| continue | ||
| } | ||
|
|
||
| if len(addresses) > 0 && !includes(addresses, log.Address) { | ||
| continue | ||
| } | ||
| // If the to filtered topics is greater than the amount of topics in logs, skip. | ||
| if len(topics) > len(log.Topics) { | ||
| continue | ||
| } | ||
| for i, sub := range topics { | ||
| match := len(sub) == 0 // empty rule set == wildcard | ||
| for _, topic := range sub { | ||
| if log.Topics[i] == topic { | ||
| match = true | ||
| break | ||
| } | ||
| } | ||
| if !match { | ||
| continue Logs | ||
| } | ||
| } | ||
| ret = append(ret, log) | ||
| } | ||
| return ret | ||
| } | ||
|
|
||
| func includes(addresses []common.Address, a common.Address) bool { | ||
| for _, addr := range addresses { | ||
| if addr == a { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
The filterLogs and includes functions are utility functions used to filter logs based on certain criteria. Ensure that these functions are covered by unit tests to validate their correctness and performance, especially since they are likely to be called with a large number of logs.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1735 +/- ##
===================================================
- Coverage 47.05001% 47.04104% -0.00897%
===================================================
Files 384 401 +17
Lines 29373 30754 +1381
Branches 83 83
===================================================
+ Hits 13820 14467 +647
- Misses 14102 14765 +663
- Partials 1451 1522 +71
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between ecbedad and 37247198ef90e9b87d82251add2675acda2a922b.Files selected for processing (1)
- services/omnirpc/proxy/simpleproxy.go (1 hunks)
Additional comments: 2
services/omnirpc/proxy/simpleproxy.go (2)
- 91-91: The
ProxyRequestmethod does not support batch requests, which is indicated by the error on line 91. Ensure that this limitation is documented in the user-facing API documentation.- 465-509: The
filterLogsandincludesfunctions are utility functions used to filter logs based on certain criteria. Ensure that these functions are covered by unit tests to validate their correctness and performance, especially since they are likely to be called with a large number of logs.
| proxyURL string | ||
| } | ||
|
|
||
| // NewSimpleProxy creates a new simply proxy |
There was a problem hiding this comment.
The comment on line 43 has a typo: "simply" should be "simple".
- // NewSimpleProxy creates a new simply proxy
+ // NewSimpleProxy creates a new simple proxyCommittable 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.
| // NewSimpleProxy creates a new simply proxy | |
| // NewSimpleProxy creates a new simple proxy |
| func (r *SimpleProxy) getHarmonyReceiptVerify(parentCtx context.Context, txHash common.Hash, rawBody []byte, checkVersion bool) (_ []byte, err error) { | ||
| ctx, span := r.tracer.Start(parentCtx, "getHarmonyReceiptVerify") | ||
|
|
||
| defer func() { | ||
| metrics.EndSpanWithErr(span, err) | ||
| }() | ||
|
|
||
| hmyClient, err := client.DialHarmonyBackend(ctx, r.proxyURL, r.handler, client.Capture(true)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not dial harmony backend: %w", err) | ||
| } | ||
|
|
||
| var harmonyReceipt, ethReceipt *types.Receipt | ||
| var rawResp []byte | ||
| g, gCtx := errgroup.WithContext(ctx) | ||
|
|
||
| g.Go(func() error { | ||
| harmonyReceipt, err = hmyClient.HarmonyTransactionReceipt(gCtx, txHash) |
There was a problem hiding this comment.
The getHarmonyReceiptVerify method performs parallel requests to verify Harmony transaction receipts. It is crucial to handle potential data races when accessing shared resources in goroutines, such as err in line 255. Use a local variable for error handling within each goroutine to avoid data races.
- harmonyReceipt, err = hmyClient.HarmonyTransactionReceipt(gCtx, txHash)
+ harmonyReceipt, goroutineErr := hmyClient.HarmonyTransactionReceipt(gCtx, txHash)
+ if goroutineErr != nil {
+ return fmt.Errorf("could not get harmony receipt: %w", goroutineErr)
+ }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.
| func (r *SimpleProxy) getHarmonyReceiptVerify(parentCtx context.Context, txHash common.Hash, rawBody []byte, checkVersion bool) (_ []byte, err error) { | |
| ctx, span := r.tracer.Start(parentCtx, "getHarmonyReceiptVerify") | |
| defer func() { | |
| metrics.EndSpanWithErr(span, err) | |
| }() | |
| hmyClient, err := client.DialHarmonyBackend(ctx, r.proxyURL, r.handler, client.Capture(true)) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not dial harmony backend: %w", err) | |
| } | |
| var harmonyReceipt, ethReceipt *types.Receipt | |
| var rawResp []byte | |
| g, gCtx := errgroup.WithContext(ctx) | |
| g.Go(func() error { | |
| harmonyReceipt, err = hmyClient.HarmonyTransactionReceipt(gCtx, txHash) | |
| harmonyReceipt, goroutineErr := hmyClient.HarmonyTransactionReceipt(gCtx, txHash) | |
| if goroutineErr != nil { | |
| return fmt.Errorf("could not get harmony receipt: %w", goroutineErr) | |
| } |
There was a problem hiding this comment.
these will get shadoed anway so this won't interfere wiht parent, right?
There was a problem hiding this comment.
@trajan0x, you are correct that the goroutineErr variable declared within the goroutine will be local to that goroutine and will not interfere with the err variable in the parent scope. In Go, each goroutine operates with its own stack, and variables declared within a goroutine are scoped to that goroutine. This means that goroutineErr is completely independent of the err variable used outside the goroutine, effectively preventing any potential data races or interference between them.
This approach ensures that error handling within each goroutine is isolated, enhancing the robustness of the error management strategy in concurrent environments. If you have any more questions or need further clarification, feel free to ask!
| func (r *SimpleProxy) getLogsHarmonyVerify(parentCtx context.Context, query ethereum.FilterQuery, rawBody []byte) (rawResp []byte, err error) { | ||
| ctx, span := r.tracer.Start(parentCtx, "getLogsHarmonyVerify") | ||
|
|
||
| defer func() { | ||
| metrics.EndSpanWithErr(span, err) | ||
| }() | ||
|
|
||
| hmyClient, err := client.DialHarmonyBackend(ctx, r.proxyURL, r.handler, client.Capture(true)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not dial harmony backend: %w", err) | ||
| } | ||
|
|
||
| var ethLogs []types.Log | ||
| var rpcMessage JSONRPCMessage | ||
|
|
||
| g, gCtx := errgroup.WithContext(ctx) | ||
| g.Go(func() error { | ||
| web3Version, err := hmyClient.Web3Version(gCtx) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get web3 version: %w", err) | ||
| } | ||
|
|
||
| if !strings.Contains(web3Version, expectedVersion) { | ||
| return fmt.Errorf("expected version %s, got %s", expectedVersion, web3Version) | ||
| } | ||
| return nil | ||
| }) | ||
|
|
||
| g.Go(func() error { | ||
| rawResp, err = r.makeReq(ctx, rawBody) | ||
| if err != nil { | ||
| return fmt.Errorf("could not make req: %w", err) | ||
| } | ||
|
|
||
| err = json.Unmarshal(rawResp, &rpcMessage) | ||
| if err != nil { | ||
| return fmt.Errorf("could not unmarshal: %w", err) | ||
| } | ||
|
|
||
| err = json.Unmarshal(rpcMessage.Result, ðLogs) | ||
| if err != nil { | ||
| return fmt.Errorf("could not unmarshal eth receipt: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
|
|
||
| err = g.Wait() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get logs: %w", err) | ||
| } | ||
|
|
||
| uniqueHashes := sets.NewString() | ||
| for i := 0; i < len(ethLogs); i++ { | ||
| uniqueHashes.Insert(ethLogs[i].TxHash.String()) | ||
| } | ||
|
|
||
| g, gCtx = errgroup.WithContext(ctx) | ||
| var logs []*types.Log | ||
| var mux sync.Mutex | ||
| for _, hash := range uniqueHashes.List() { | ||
| g.Go(func() error { | ||
| rawReqBody, err := json.Marshal(rpc.Request{ | ||
| ID: 1, | ||
| Method: client.TransactionReceiptByHashMethod.String(), | ||
| Params: []json.RawMessage{json.RawMessage(fmt.Sprintf("\"%s\"", hash))}, | ||
| }) | ||
|
|
||
| resp, err := r.getHarmonyReceiptVerify(gCtx, common.HexToHash(hash), rawReqBody, false) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get harmony receipt: %w", err) | ||
| } | ||
|
|
||
| var rpcMessage JSONRPCMessage | ||
| err = json.Unmarshal(resp, &rpcMessage) | ||
| if err != nil { | ||
| return fmt.Errorf("could not unmarshal: %w", err) | ||
| } | ||
|
|
||
| var receipt types.Receipt | ||
| err = json.Unmarshal(rpcMessage.Result, &receipt) | ||
| if err != nil { | ||
| return fmt.Errorf("could not unmarshal: %w", err) | ||
| } | ||
|
|
||
| mux.Lock() | ||
| logs = append(logs, receipt.Logs...) | ||
| mux.Unlock() | ||
| return nil | ||
| }) | ||
| } | ||
|
|
||
| err = g.Wait() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get logs: %w", err) | ||
| } | ||
|
|
||
| filteredLogs := filterLogs(logs, query.FromBlock, query.ToBlock, query.Addresses, query.Topics) | ||
| if err != nil { |
There was a problem hiding this comment.
The getLogsHarmonyVerify method contains a potential error handling issue. The error check on line 453 is redundant and will never be true because err is not reassigned after its declaration. This check should be removed or corrected to check the actual error returned from the filterLogs function.
- if err != nil {
+ // Assuming filterLogs returns an error, which is not shown in the current context.
+ filteredLogs, filterErr := filterLogs(logs, query.FromBlock, query.ToBlock, query.Addresses, query.Topics)
+ if filterErr != nil {
return nil, fmt.Errorf("could not filter logs: %w", err)
}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.
| func (r *SimpleProxy) getLogsHarmonyVerify(parentCtx context.Context, query ethereum.FilterQuery, rawBody []byte) (rawResp []byte, err error) { | |
| ctx, span := r.tracer.Start(parentCtx, "getLogsHarmonyVerify") | |
| defer func() { | |
| metrics.EndSpanWithErr(span, err) | |
| }() | |
| hmyClient, err := client.DialHarmonyBackend(ctx, r.proxyURL, r.handler, client.Capture(true)) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not dial harmony backend: %w", err) | |
| } | |
| var ethLogs []types.Log | |
| var rpcMessage JSONRPCMessage | |
| g, gCtx := errgroup.WithContext(ctx) | |
| g.Go(func() error { | |
| web3Version, err := hmyClient.Web3Version(gCtx) | |
| if err != nil { | |
| return fmt.Errorf("could not get web3 version: %w", err) | |
| } | |
| if !strings.Contains(web3Version, expectedVersion) { | |
| return fmt.Errorf("expected version %s, got %s", expectedVersion, web3Version) | |
| } | |
| return nil | |
| }) | |
| g.Go(func() error { | |
| rawResp, err = r.makeReq(ctx, rawBody) | |
| if err != nil { | |
| return fmt.Errorf("could not make req: %w", err) | |
| } | |
| err = json.Unmarshal(rawResp, &rpcMessage) | |
| if err != nil { | |
| return fmt.Errorf("could not unmarshal: %w", err) | |
| } | |
| err = json.Unmarshal(rpcMessage.Result, ðLogs) | |
| if err != nil { | |
| return fmt.Errorf("could not unmarshal eth receipt: %w", err) | |
| } | |
| return nil | |
| }) | |
| err = g.Wait() | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get logs: %w", err) | |
| } | |
| uniqueHashes := sets.NewString() | |
| for i := 0; i < len(ethLogs); i++ { | |
| uniqueHashes.Insert(ethLogs[i].TxHash.String()) | |
| } | |
| g, gCtx = errgroup.WithContext(ctx) | |
| var logs []*types.Log | |
| var mux sync.Mutex | |
| for _, hash := range uniqueHashes.List() { | |
| g.Go(func() error { | |
| rawReqBody, err := json.Marshal(rpc.Request{ | |
| ID: 1, | |
| Method: client.TransactionReceiptByHashMethod.String(), | |
| Params: []json.RawMessage{json.RawMessage(fmt.Sprintf("\"%s\"", hash))}, | |
| }) | |
| resp, err := r.getHarmonyReceiptVerify(gCtx, common.HexToHash(hash), rawReqBody, false) | |
| if err != nil { | |
| return fmt.Errorf("could not get harmony receipt: %w", err) | |
| } | |
| var rpcMessage JSONRPCMessage | |
| err = json.Unmarshal(resp, &rpcMessage) | |
| if err != nil { | |
| return fmt.Errorf("could not unmarshal: %w", err) | |
| } | |
| var receipt types.Receipt | |
| err = json.Unmarshal(rpcMessage.Result, &receipt) | |
| if err != nil { | |
| return fmt.Errorf("could not unmarshal: %w", err) | |
| } | |
| mux.Lock() | |
| logs = append(logs, receipt.Logs...) | |
| mux.Unlock() | |
| return nil | |
| }) | |
| } | |
| err = g.Wait() | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get logs: %w", err) | |
| } | |
| filteredLogs := filterLogs(logs, query.FromBlock, query.ToBlock, query.Addresses, query.Topics) | |
| if err != nil { | |
| filteredLogs, filterErr := filterLogs(logs, query.FromBlock, query.ToBlock, query.Addresses, query.Topics) | |
| if filterErr != nil { |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between 37247198ef90e9b87d82251add2675acda2a922b and a319d29b384acf707a1738bd8310e84e08664ebd.Files selected for processing (1)
- ethergo/client/client.go (3 hunks)
Additional comments: 2
ethergo/client/client.go (2)
- 53-54: The addition of the
Web3Versionmethod to theEVMinterface is a necessary update for supporting Harmony blockchain interactions. This method will allow the client to retrieve the web3 version, which is a fundamental aspect of interacting with Ethereum and Harmony blockchains.- 76-83: The update to the
DialBackendmethod in theclientImplstruct, specifically the assignment toclient.rpcClient, is crucial for ensuring that the RPC client is properly initialized. This change is necessary for theWeb3Versionmethod and potentially other RPC interactions to function correctly.
| // Web3Version calls Web3Version on the underlying client | ||
| // | ||
| //nolint:wrapcheck | ||
| func (c *clientImpl) Web3Version(ctx context.Context) (version string, err error) { | ||
| requestCtx, span := c.startSpan(ctx, Web3VersionMethod) | ||
| defer func() { | ||
| metrics.EndSpanWithErr(span, err) | ||
| }() | ||
|
|
||
| var ver string | ||
| if err := c.rpcClient.CallContext(requestCtx, &ver, Web3VersionMethod.String()); err != nil { | ||
| return "", err | ||
| } | ||
| return ver, nil | ||
| } |
There was a problem hiding this comment.
The implementation of the Web3Version method in the clientImpl struct correctly calls the underlying RPC client to retrieve the web3 version. The use of CallContext for making the RPC call is appropriate, and the error handling is consistent with the rest of the file. However, consider adding a brief comment above the Web3Version method to explain its purpose, especially for developers unfamiliar with web3 or the specific needs of Harmony blockchain interactions.
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.
| // Web3Version calls Web3Version on the underlying client | |
| // | |
| //nolint:wrapcheck | |
| func (c *clientImpl) Web3Version(ctx context.Context) (version string, err error) { | |
| requestCtx, span := c.startSpan(ctx, Web3VersionMethod) | |
| defer func() { | |
| metrics.EndSpanWithErr(span, err) | |
| }() | |
| var ver string | |
| if err := c.rpcClient.CallContext(requestCtx, &ver, Web3VersionMethod.String()); err != nil { | |
| return "", err | |
| } | |
| return ver, nil | |
| } | |
| // Web3Version calls Web3Version on the underlying client | |
| // | |
| //nolint:wrapcheck | |
| func (c *clientImpl) Web3Version(ctx context.Context) (version string, err error) { | |
| requestCtx, span := c.startSpan(ctx, Web3VersionMethod) | |
| defer func() { | |
| metrics.EndSpanWithErr(span, err) | |
| }() | |
| var ver string | |
| if err := c.rpcClient.CallContext(requestCtx, &ver, Web3VersionMethod.String()); err != nil { | |
| return "", err | |
| } | |
| return ver, nil | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between a319d29b384acf707a1738bd8310e84e08664ebd and 7964800ef04f96c71e302d0a92b2efedb2bde820.Files ignored due to path filters (1)
services/stiprelayer/go.modis excluded by:!**/*.mod
Files selected for processing (1)
- services/omnirpc/proxy/simpleproxy.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/omnirpc/proxy/simpleproxy.go
trajan0x
force-pushed
the
feat/omnirpc-double
branch
from
7964800
to
89dee12
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- ethergo/client/client.go (3 hunks)
- ethergo/client/hmy.go (1 hunks)
- ethergo/client/methods.go (1 hunks)
Additional comments: 14
ethergo/client/hmy.go (7)
- 14-21: The HarmonyVM interface correctly extends the EVM interface with Harmony-specific methods. This design follows the principle of interface segregation, ensuring that Harmony-specific functionalities are encapsulated within this interface.
- 23-25: The harmonyClient struct is correctly defined to embed the clientImpl struct, allowing it to inherit methods from the Ethereum client implementation while also implementing the HarmonyVM interface.
- 27-38: The DialHarmonyBackend function is implemented correctly, ensuring that it returns an error if the underlying DialBackend call fails or if the type assertion fails. This ensures robust error handling and clear error messages for the caller.
- 41-53: The FilterHarmonyLogs method correctly uses the startSpan method for tracing and metrics.EndSpanWithErr for error handling in the deferred function. The use of CallContext for making the RPC call is appropriate. However, ensure that the HarmonyGetLogsMethod is correctly implemented and registered in the RPC client.
- 56-72: The HarmonyTransactionReceipt method follows the same pattern as FilterHarmonyLogs for tracing and error handling. The check for a nil receipt before returning ethereum.NotFound is a good practice to avoid returning misleading errors when the receipt is simply not found.
- 75-93: The toFilterArg function correctly handles the conversion of an ethereum.FilterQuery to the format expected by the Harmony RPC methods. The error handling for mutually exclusive parameters (BlockHash and FromBlock/ToBlock) is correctly implemented.
- 96-104: The toBlockNumArg function correctly handles the conversion of a block number to the string format expected by the RPC methods, including special cases for nil (latest) and -1 (pending).
ethergo/client/methods.go (3)
- 64-71: The Harmony-specific RPC methods (HarmonyGetLogsMethod and HarmonyGetReceiptMethod) are correctly defined as constants of type RPCMethod. This ensures consistency with the existing Ethereum RPC methods.
- 76-77: The addition of Web3VersionMethod to the NetMethods section is correctly implemented, following the pattern of other method definitions.
- 83-83: The allMethods slice is correctly updated to include the new Harmony RPC methods and the Web3VersionMethod. This ensures that these methods are recognized and can be used throughout the client library.
ethergo/client/client.go (4)
- 53-54: The addition of the Web3Version method to the EVM interface is correctly implemented, ensuring that implementations of the EVM interface provide a method to retrieve the web3 version.
- 62-62: The rpcClient field is correctly added to the clientImpl struct, allowing the struct to hold a reference to the RPC client used for making calls to the Ethereum node.
- 77-84: The DialBackend function is correctly updated to assign the rpcClient field of the clientImpl struct. This ensures that the rpcClient is available for use in methods that require making RPC calls.
- 233-247: The implementation of the Web3Version method in the clientImpl struct correctly calls the underlying RPC client to retrieve the web3 version. The use of CallContext for making the RPC call is appropriate, and the error handling is consistent with the rest of the file.
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Deploying sanguine-fe with
|
| Latest commit: |
a76559c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://70bcfdea.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-omnirpc-double.sanguine-fe.pages.dev |
github-actions
bot
removed
needs-go-generate-services/rfq
needs-go-generate-agents
Stale
labels
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
| // Web3Version is not implemented on this backend. | ||
| func (s Client) Web3Version(_ context.Context) (version string, err error) { | ||
| // TODO implement me | ||
| panic("cannot implement on this backend") | ||
| } |
There was a problem hiding this comment.
Method Web3Version is added but not implemented and will panic if called. Ensure this method is not used in production and consider documenting its non-implementation clearly.
| // FeeHistory is not implemented on this backend. | ||
| func (s Client) FeeHistory(ctx context.Context, blockCount uint64, lastBlock *big.Int, rewardPercentiles []float64) (*ethereum.FeeHistory, error) { | ||
| func (s Client) FeeHistory(_ context.Context, _ uint64, _ *big.Int, _ []float64) (*ethereum.FeeHistory, error) { |
There was a problem hiding this comment.
Method FeeHistory is similarly not implemented and will panic if called. As with Web3Version, ensure it is not used where it can cause runtime errors and document its limitations.
github-actions
bot
added
needs-go-generate-contrib/promexporter
and removed
needs-go-generate-ethergo
labels
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
New Features
Enhancements