-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pass the gateway config to NetAPI
so it can return the proper network ID
#311
Pass the gateway config to NetAPI
so it can return the proper network ID
#311
Conversation
WalkthroughThe changes integrate the configuration parameter into various components, ensuring the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Config
participant NetAPI
Client ->> API: Call SupportedAPIs(..., config)
API ->> NetAPI: Initialize with NewNetAPI(config)
NetAPI ->> Config: Get EVMNetworkID
Config -->> NetAPI: Return EVMNetworkID
NetAPI -->> Client: Return network ID
Assessment against linked issues
Poem
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- api/api.go (2 hunks)
- api/net.go (2 hunks)
- bootstrap/bootstrap.go (1 hunks)
- tests/e2e_web3js_test.go (1 hunks)
- tests/web3js/eth_non_interactive_test.js (1 hunks)
- tests/web3js/net_namespace_test.js (1 hunks)
Additional comments not posted (8)
tests/web3js/net_namespace_test.js (3)
5-8
: Approved: Correct implementation of network ID test.This test case correctly checks if the network ID returned by
web3.eth.net.getId()
matches the expected value, aligning with the PR's objective to dynamically provide the correct network ID.
10-13
: Approved: Network listening status test.The test correctly asserts that the network is always listening, which is a fundamental aspect of the network's functionality.
15-18
: Approved: Peer count test.This test case correctly checks the peer count. Ensure that the test environment is configured to have exactly one peer, as expected by this test.
api/net.go (2)
11-19
: Approved: Correct implementation ofNetAPI
with configuration.The
NetAPI
struct is correctly defined with a configuration field, and the constructorNewNetAPI
properly initializes this field. This setup is crucial for the dynamic retrieval of network IDs.
34-34
: Approved: Dynamic network ID retrieval inVersion
method.The
Version
method correctly retrieves the network ID from the configuration, replacing the previously hardcoded value. This change aligns perfectly with the PR's objectives.tests/e2e_web3js_test.go (1)
21-23
: Approved: Addition of new test case fornet_*
JSON-RPC endpoints.The inclusion of the
net_namespace_test.js
in the test suite is a crucial step for ensuring that the new network functionality is thoroughly tested. This aligns with the PR's objectives of enhancing test coverage.tests/web3js/eth_non_interactive_test.js (1)
193-193
: Approved: Updated expected result fornet_version
in batch request test.The update to the expected result for the
net_version
method in the batch request test case correctly reflects the new dynamic configuration capability. This ensures that the test remains accurate and aligns with the PR's objectives.api/api.go (1)
Line range hint
36-52
: Approval: UpdatedSupportedAPIs
to include configuration.The update to include a
config
parameter in theSupportedAPIs
function and the initialization ofNetAPI
with this configuration are in line with the PR objectives. This allows theNetAPI
to dynamically use the network ID from the configuration, addressing the issue of a hardcoded network ID. Ensure that all calls toSupportedAPIs
across the codebase are updated to pass the new parameter.
supportedAPIs := api.SupportedAPIs( | ||
blockchainAPI, | ||
streamAPI, | ||
pullAPI, | ||
debugAPI, | ||
cfg, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor suggestion: Pass configuration to SupportedAPIs
method.
The change to pass the cfg
object to SupportedAPIs
aligns with the PR objectives to make the network ID dynamic based on configuration. This is a crucial change for the flexibility of the NetAPI
. Ensure that all dependencies and usages of SupportedAPIs
have been updated accordingly to handle the new parameter.
Closes: #310
Description
Until now, the
net_version
was hard-coded to return the testnet chain ID.For contributor use:
master
branchFiles changed
in the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes
Tests
net_*
JSON-RPC endpoints and network information retrieval usingweb3.eth.net
.