Arbitrum SDK #1116
Arbitrum SDK #1116
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1116 +/- ##
===================================================
- Coverage 46.18948% 43.79835% -2.39114%
===================================================
Files 88 166 +78
Lines 4317 11763 +7446
Branches 79 79
===================================================
+ Hits 1994 5152 +3158
- Misses 2155 5997 +3842
- Partials 168 614 +446
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
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. |
|
Warning
|
| Files | Summary |
|---|---|
.gitmodules, ethergo/internal/nitro-contracts |
Added nitro-contracts submodule under ethergo/internal. |
ethergo/chain/README.md |
Marked Chain module as deprecated. |
ethergo/parser/abiutil/selector.go, ethergo/parser/abiutil/selector_test.go |
Enhanced ABI utilities with new function and tests. |
ethergo/sdks/arbitrum/contracts/... (multiple files) |
Added and updated files for Arbitrum SDK, including auto-generated Go bindings, metadata handling, and mock implementations for ArbGasInfo and NodeInterface contracts. |
ethergo/sdks/arbitrum/doc.go, ethergo/sdks/arbitrum/internal/..., ethergo/sdks/arbitrum/options.go, ethergo/sdks/arbitrum/sdk.go |
Introduced and updated internal mechanisms and SDK functionalities for Arbitrum. |
ethergo/sdks/doc.go |
Introduced a package for chain-specific SDKs. |
ethergo/submitter/README.md |
Updated visual representation settings in dark mode. |
🐰✨
In the world of code and nightly builds,
A rabbit hopped through, making swift tilts.
With updates and patches, it danced around,
Bringing new features, where solutions are found.
🌟🚀
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?
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>.Generate unit-tests for this file.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit tests for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin 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 generate interesting stats about this repository from git and render them as a table.@coderabbitai show all the console.log statements in this repository.@coderabbitai read src/utils.ts and generate unit tests.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
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)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger a review. This is useful when automatic reviews are disabled for the repository.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai helpto get help.
Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
CodeRabbit Configration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - The JSON schema for the configuration file is available here.
- 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/coderabbit-overrides.v2.json
CodeRabbit Discord Community
Join our Discord Community to get help, request features, and share feedback.
github-actions
bot
added
needs-go-generate-services/rfq
needs-go-generate-agents
and removed
needs-go-generate-contrib/promexporter
labels
Qodana for Go7 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
![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: 10
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
ethergo/sdks/arbitrum/contracts/arbgasinfo/arbgasinfo.contractinfo.jsonis excluded by:!**/*.jsonethergo/sdks/arbitrum/contracts/nodeinterface/nodeinterface.contractinfo.jsonis excluded by:!**/*.json
Files selected for processing (28)
- .gitmodules (1 hunks)
- ethergo/chain/README.md (1 hunks)
- ethergo/internal/nitro-contracts (1 hunks)
- ethergo/parser/abiutil/selector.go (2 hunks)
- ethergo/parser/abiutil/selector_test.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/arbgasinfo/arbgasinfo.abigen.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/arbgasinfo/arbgasinfo.metadata.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/arbgasinfo/doc.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/arbgasinfo/generate.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/arbgasinfo/helpers.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/arbgasinfo/icaller_generated.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/arbgasinfo/mocks/i_arb_gas_info_caller.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/doc.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/nodeinterface/doc.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/nodeinterface/generate.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/nodeinterface/helpers.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/nodeinterface/icaller_generated.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/nodeinterface/mocks/i_node_interface.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/nodeinterface/nodeinterface.abigen.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/nodeinterface/nodeinterface.metadata.go (1 hunks)
- ethergo/sdks/arbitrum/contracts/nodeinterface/suite_test.go (1 hunks)
- ethergo/sdks/arbitrum/doc.go (1 hunks)
- ethergo/sdks/arbitrum/internal/addresses.go (1 hunks)
- ethergo/sdks/arbitrum/internal/doc.go (1 hunks)
- ethergo/sdks/arbitrum/options.go (1 hunks)
- ethergo/sdks/arbitrum/sdk.go (1 hunks)
- ethergo/sdks/doc.go (1 hunks)
- ethergo/submitter/README.md (2 hunks)
Files skipped from review due to trivial changes (7)
- ethergo/internal/nitro-contracts
- ethergo/sdks/arbitrum/contracts/arbgasinfo/doc.go
- ethergo/sdks/arbitrum/contracts/arbgasinfo/icaller_generated.go
- ethergo/sdks/arbitrum/contracts/doc.go
- ethergo/sdks/arbitrum/contracts/nodeinterface/icaller_generated.go
- ethergo/sdks/arbitrum/internal/doc.go
- ethergo/submitter/README.md
Additional comments: 19
ethergo/chain/README.md (1)
- 1-5: LGTM!
ethergo/sdks/arbitrum/internal/addresses.go (1)
- 5-13: Ensure the hardcoded addresses ("0x6e" and "0xc8") are correct and intended for use across all environments. Consider adding comments to explain the choice of these specific addresses.
ethergo/sdks/arbitrum/contracts/arbgasinfo/arbgasinfo.metadata.go (1)
- 1-25: LGTM!
ethergo/sdks/arbitrum/contracts/nodeinterface/nodeinterface.metadata.go (1)
- 1-25: LGTM!
ethergo/sdks/arbitrum/contracts/nodeinterface/doc.go (1)
- 1-8: LGTM!
ethergo/sdks/arbitrum/contracts/arbgasinfo/helpers.go (1)
- 1-38: LGTM!
.gitmodules (1)
- 14-16: LGTM!
ethergo/sdks/arbitrum/options.go (1)
- 1-40: LGTM!
ethergo/sdks/arbitrum/contracts/nodeinterface/suite_test.go (1)
- 1-69: LGTM!
ethergo/parser/abiutil/selector.go (1)
- 64-77: LGTM!
ethergo/parser/abiutil/selector_test.go (1)
- 30-33: LGTM!
ethergo/sdks/arbitrum/contracts/nodeinterface/helpers.go (1)
- 1-115: LGTM!
ethergo/sdks/arbitrum/contracts/nodeinterface/mocks/i_node_interface.go (1)
- 1-282: LGTM!
ethergo/sdks/arbitrum/contracts/arbgasinfo/mocks/i_arb_gas_info_caller.go (1)
- 14-555: Ensure all newly introduced methods in the mock file are correctly implemented according to the interface they mock. This includes correct parameters, return types, and mock call patterns. The use of
mock.Calledand type assertions appears consistent and correctly implemented across methods.ethergo/sdks/arbitrum/contracts/nodeinterface/nodeinterface.abigen.go (3)
- 31-44: The metadata for the NodeInterface contract, including ABI and method signatures, is correctly defined. This ensures that the Go binding can correctly interact with the contract based on its ABI.
- 113-156: The functions for creating new instances of the contract bindings (
NewNodeInterface,NewNodeInterfaceCaller, etc.) are correctly implemented, ensuring that users can interact with the contract in various ways (read, write, filter events) depending on their needs.- 158-480: The method bindings for contract interactions, including both read (call) and write (transact) operations, are correctly implemented. Each method correctly translates Go arguments to Ethereum call or transaction parameters and handles return values appropriately. Ensure that the method names and parameters accurately reflect the Solidity contract's functions they are intended to interact with.
ethergo/sdks/arbitrum/contracts/arbgasinfo/arbgasinfo.abigen.go (2)
- 205-218: The method
GetAmortizedCostCapBipsis correctly implemented for calling a contract method. Verify that all contract method calls throughout this file handle errors appropriately and convert the output types correctly as demonstrated here.- 686-699: In
GetPricesInWeiWithAggregator, ensure that the handling of theaggregatorparameter is secure and that there are no potential issues with passing user-controlled addresses to contract methods.
| // Deprecated: Use ArbGasInfoMetaData.ABI instead. | ||
| var ArbGasInfoABI = ArbGasInfoMetaData.ABI |
There was a problem hiding this comment.
Deprecated items should be clearly documented, explaining why they are deprecated and what should be used instead. Ensure the documentation for ArbGasInfoABI includes this information.
| // Deprecated: Use ArbGasInfoMetaData.Sigs instead. | ||
| // ArbGasInfoFuncSigs maps the 4-byte function signature to its string representation. | ||
| var ArbGasInfoFuncSigs = ArbGasInfoMetaData.Sigs |
There was a problem hiding this comment.
Similar to the previous comment, ensure that ArbGasInfoFuncSigs is also clearly documented with information on its deprecation and guidance on what should be used instead.
| func NewArbGasInfo(address common.Address, backend bind.ContractBackend) (*ArbGasInfo, error) { | ||
| contract, err := bindArbGasInfo(address, backend, backend, backend) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &ArbGasInfo{ArbGasInfoCaller: ArbGasInfoCaller{contract: contract}, ArbGasInfoTransactor: ArbGasInfoTransactor{contract: contract}, ArbGasInfoFilterer: ArbGasInfoFilterer{contract: contract}}, nil |
There was a problem hiding this comment.
When creating a new instance of ArbGasInfo, error handling is correctly implemented. However, consider adding a comment to explain the significance of binding the contract to different interfaces (Caller, Transactor, Filterer) for clarity to developers.
| func bindArbGasInfo(address common.Address, caller bind.ContractCaller, transactor bind.ContractTransactor, filterer bind.ContractFilterer) (*bind.BoundContract, error) { | ||
| parsed, err := abi.JSON(strings.NewReader(ArbGasInfoABI)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return bind.NewBoundContract(address, parsed, caller, transactor, filterer), nil |
There was a problem hiding this comment.
The bindArbGasInfo function correctly binds a generic wrapper to an already deployed contract. Ensure that error messages returned from this function are descriptive enough to help developers diagnose issues during the binding process.
| func (_ArbGasInfo *ArbGasInfoRaw) Call(opts *bind.CallOpts, result *[]interface{}, method string, params ...interface{}) error { | ||
| return _ArbGasInfo.Contract.ArbGasInfoCaller.contract.Call(opts, result, method, params...) | ||
| } | ||
|
|
||
| // Transfer initiates a plain transaction to move funds to the contract, calling | ||
| // its default method if one is available. | ||
| func (_ArbGasInfo *ArbGasInfoRaw) Transfer(opts *bind.TransactOpts) (*types.Transaction, error) { | ||
| return _ArbGasInfo.Contract.ArbGasInfoTransactor.contract.Transfer(opts) | ||
| } | ||
|
|
||
| // Transact invokes the (paid) contract method with params as input values. | ||
| func (_ArbGasInfo *ArbGasInfoRaw) Transact(opts *bind.TransactOpts, method string, params ...interface{}) (*types.Transaction, error) { | ||
| return _ArbGasInfo.Contract.ArbGasInfoTransactor.contract.Transact(opts, method, params...) | ||
| } |
There was a problem hiding this comment.
For methods Call, Transfer, and Transact in ArbGasInfoRaw, ensure that the documentation comments clearly describe the purpose of these methods and how they should be used, especially highlighting the difference between Call (for constant methods) and Transact (for state-changing methods).
| @@ -0,0 +1,2 @@ | |||
| // Package arbitrum provides the csutom arbitrum sdk functionality | |||
There was a problem hiding this comment.
Typo in package description comment: "csutom" should be "custom".
-// Package arbitrum provides the csutom arbitrum sdk functionality
+// Package arbitrum provides the custom arbitrum sdk functionalityCommittable 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.
| // Package arbitrum provides the csutom arbitrum sdk functionality | |
| // Package arbitrum provides the custom arbitrum sdk functionality |
| @@ -0,0 +1,2 @@ | |||
| // Package sdks provides chain-specific sdks fro different evm blockchains. These should track things like custom finality periods based on l1, custom gas pricing logic, etc. | |||
There was a problem hiding this comment.
Typo in package description comment: "fro" should be "for".
-// Package sdks provides chain-specific sdks fro different evm blockchains. These should track things like custom finality periods based on l1, custom gas pricing logic, etc.
+// Package sdks provides chain-specific sdks for different evm blockchains. These should track things like custom finality periods based on l1, custom gas pricing logic, etc.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.
| // Package sdks provides chain-specific sdks fro different evm blockchains. These should track things like custom finality periods based on l1, custom gas pricing logic, etc. | |
| // Package sdks provides chain-specific sdks for different evm blockchains. These should track things like custom finality periods based on l1, custom gas pricing logic, etc. |
| package arbgasinfo | ||
|
|
||
| //go:generate go run github.com/synapsecns/sanguine/tools/abigen generate --sol ../../../../internal/nitro-contracts/src/precompiles/ArbGasInfo.sol --pkg arbgasinfo --sol-version 0.8.17 --filename arbgasinfo | ||
| // here we generate some interfaces we use in for our mocks. TODO this should be automated in abigen for all contracts + be condensed |
There was a problem hiding this comment.
Address the TODO comment regarding the automation of interface generation in abigen. Consider implementing the automation or creating a tracking issue to address this in the future.
Would you like me to open a GitHub issue to track this task?
| package nodeinterface | ||
|
|
||
| //go:generate go run github.com/synapsecns/sanguine/tools/abigen generate --sol ../../../../internal/nitro-contracts/src/node-interface/NodeInterface.sol --pkg nodeinterface --sol-version 0.8.17 --filename nodeinterface | ||
| // here we generate some interfaces we use in for our mocks. TODO this should be automated in abigen for all contracts + be condensed |
There was a problem hiding this comment.
Address the TODO comment regarding the automation of interface generation in abigen. Consider implementing the automation or creating a tracking issue to address this in the future.
Would you like me to open a GitHub issue to track this task?
| if call.To == nil { | ||
| return 0, errors.New("call.To cannot be nil") | ||
| } | ||
| // TODO: maybe need to copy the logic that sets the gasprice if it's empty? |
There was a problem hiding this comment.
Address the TODO comment regarding potentially copying logic for setting the gas price if empty. Consider implementing the logic or creating a tracking issue to address this in the future.
Would you like me to open a GitHub issue to track this task?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (5)
- agents/agents/executor/executor_test.go (2 hunks)
- agents/agents/guard/fraud_test.go (2 hunks)
- ethergo/backends/anvil/suite_test.go (2 hunks)
- ethergo/sdks/arbitrum/contracts/nodeinterface/suite_test.go (1 hunks)
- ethergo/sdks/arbitrum/sdk.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- ethergo/sdks/arbitrum/contracts/nodeinterface/suite_test.go
- ethergo/sdks/arbitrum/sdk.go
Additional comments: 3
agents/agents/executor/executor_test.go (1)
- 20-20: The import statement for
coreis correctly added to support the use ofcore.GetEnvBoolin the conditional check. Ensure that all other imports are still necessary and remove any that are unused after this change.agents/agents/guard/fraud_test.go (2)
- 23-23: The addition of the
coreimport is appropriate for the introduced functionality.- 729-732: The conditional check to skip tests based on the
CIenvironment variable is a practical approach for handling test execution in different environments. However, ensure that skipping tests in CI does not obscure potential issues that would only surface during CI runs.
| // This test requires a call to anvil's evm.IncreaseTime() cheat code, so we should | ||
| // set up the backends with anvil. | ||
|
|
||
| // TODO: no need for this when anvil CI issues are fixed | ||
| if core.GetEnvBool("CI", false) { | ||
| return | ||
| } | ||
|
|
||
| testDone := false | ||
| defer func() { | ||
| testDone = true |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-733]
The tests are comprehensive and cover a wide range of fraud detection scenarios. Consider refactoring to improve maintainability and readability, such as breaking down large test functions into smaller, more focused tests or using helper functions for repetitive setup tasks. Additionally, ensure that all external dependencies and mock contracts are adequately documented to facilitate understanding and future updates.
| // TODO: no need for this when anvil CI issues are fixed | ||
| if core.GetEnvBool("CI", false) { | ||
| return | ||
| } |
There was a problem hiding this comment.
The conditional check to skip setup actions when running in a CI environment is implemented correctly. However, consider adding a log statement to indicate that the setup is being skipped due to the CI environment. This can improve the visibility of test behavior in CI logs.
if core.GetEnvBool("CI", false) {
+ log.Println("Skipping setup due to CI environment")
return
}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.
| // TODO: no need for this when anvil CI issues are fixed | |
| if core.GetEnvBool("CI", false) { | |
| return | |
| } | |
| // TODO: no need for this when anvil CI issues are fixed | |
| if core.GetEnvBool("CI", false) { | |
| log.Println("Skipping setup due to CI environment") | |
| return | |
| } |
| // TODO: no need for this when anvil CI issues are fixed | ||
| if core.GetEnvBool("CI", false) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Similar to the previous comment, the conditional check to skip the test suite execution in a CI environment is correctly implemented. Again, adding a log statement here would enhance visibility and debugging capabilities in CI logs.
if core.GetEnvBool("CI", false) {
+ log.Println("Skipping TestAnvilSuite due to CI environment")
return
}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.
| // TODO: no need for this when anvil CI issues are fixed | |
| if core.GetEnvBool("CI", false) { | |
| return | |
| } | |
| // TODO: no need for this when anvil CI issues are fixed | |
| if core.GetEnvBool("CI", false) { | |
| log.Println("Skipping TestAnvilSuite due to CI environment") | |
| return | |
| } |
| // TODO: no need for this when anvil CI issues are fixed | ||
| if core.GetEnvBool("CI", false) { | ||
| return | ||
| } |
There was a problem hiding this comment.
The conditional check to skip the test when running in a CI environment is implemented correctly. As with the previous file, consider adding a log statement to indicate that the test is being skipped due to the CI environment. This can aid in understanding test behavior in CI logs.
if core.GetEnvBool("CI", false) {
+ log.Println("Skipping TestSendManagerMessage due to CI environment")
return
}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.
| // TODO: no need for this when anvil CI issues are fixed | |
| if core.GetEnvBool("CI", false) { | |
| return | |
| } | |
| // TODO: no need for this when anvil CI issues are fixed | |
| if core.GetEnvBool("CI", false) { | |
| log.Println("Skipping TestSendManagerMessage due to CI environment") | |
| return | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- services/explorer/api/suite_test.go (2 hunks)
Additional comments: 1
services/explorer/api/suite_test.go (1)
- 12-13: Reordering imports is good practice for readability and maintaining a standard structure. Ensure that all necessary imports are included and unused ones are removed.
| testDone := false | ||
| defer func() { | ||
| testDone = true | ||
| }() | ||
|
|
||
| go func() { | ||
| Nil(g.T(), api.Start(g.GetTestContext(), config, g.explorerMetrics)) | ||
| err := api.Start(g.GetTestContext(), config, g.explorerMetrics) | ||
| if !testDone { | ||
| Nil(g.T(), err) | ||
| } |
There was a problem hiding this comment.
The introduction of testDone to manage goroutine execution is a practical approach. However, consider using more robust synchronization mechanisms like sync.WaitGroup or context.Context to handle goroutine lifecycles and avoid potential race conditions.
| if !testDone { | ||
| Nil(g.T(), err) |
There was a problem hiding this comment.
Conditional error handling based on testDone might mask genuine issues during the test's runtime. Consider logging the error even if testDone is true to ensure visibility of potential problems.
|
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. |
Description
Use L2 arbitrum gas estimation
Summary by CodeRabbit
nitro-contractsto enhance blockchain interaction capabilities.ArbGasInfoandNodeInterfacecontracts on the Arbitrum network, enabling advanced gas estimation and node interaction functionalities.Chainmodule README to advise against using deprecated utilities and encourage code duplication for necessary functionalities.