Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: Replace net_version with eth_chainId #2414

Closed
MartinquaXD opened this issue Feb 20, 2024 · 2 comments
Closed

chore: Replace net_version with eth_chainId #2414

MartinquaXD opened this issue Feb 20, 2024 · 2 comments
Labels
good first issue Good for newcomers

Comments

@MartinquaXD
Copy link
Contributor

Background

Some node providers don't expose the net module or node clients might not enable it by default. This causes the backend to not work with for example the public merkle RPC.

Since we are only using the net module to get the chain id (with .net().version()) we can get rid of the net dependency by using eth_chainId instead.
Note that ethcontract-rs currently also relies on net_version but a PR to address that is already available so when tackling this issue we should also bump the ethcontract-rs version.

Acceptance criteria

Service works using an RPC without net module (e.g. https://eth.merkle.io).
@MartinquaXD MartinquaXD added the good first issue Good for newcomers label Feb 20, 2024
This was referenced Feb 22, 2024
Merged
Merged
MartinquaXD added a commit that referenced this issue Feb 23, 2024
@MartinquaXD
Remove usage of net RPC module (#2425)
39d40fc 
# Description
Fixes: #2414

# Changes
* replaces all `.net().version()` calls with
`.eth().chain_id().await?.to_string()`
* adds custom clippy lint forbidding the old `version()` function
* updates the dependencies to make use of `ethcontract`
[0.25.5](https://github.com/cowprotocol/ethcontract-rs/releases/tag/v0.25.5)
which applies the same change

## How to test
Locally ran the services with `https://freerpc.merkle.io/` as the RPC
which was previously not possible because that didn't expose the `net`
module.

Checked that the clippy lint works:
```
    Checking orderbook v0.1.0 (/Users/martin/work/backend/side/crates/orderbook)
    Checking autopilot v0.1.0 (/Users/martin/work/backend/side/crates/autopilot)
warning: use of a disallowed method `web3::api::Net::version`
   --> crates/autopilot/src/run.rs:204:19
    |
204 |       let network = web3
    |  ___________________^
205 | |         .net()
206 | |         .version()
    | |__________________^
    |
    = note: Calling `eth().chain_id().await?.to_string()` is equivalent and is better supported. (from clippy.toml)
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods
    = note: `#[warn(clippy::disallowed_methods)]` on by default
```
MartinquaXD added a commit that referenced this issue Feb 23, 2024
@MartinquaXD
Drop NetworkId type from domain (#2424)
7653aac 
## Changes
`NetworkId` and `ChainId` are the same thing so it only makes sense to
keep one.
* drop `NetworkId`
* drop `Network` since it would only have 1 field
* switch internal value of `ChainId` from `U256` to `u64` since this is
guaranteed to fit per
[EIP-2294](ethereum/EIPs#2294)
* since `ChainId` is now same size as a pointer I made all functions
take an owned value instead of a reference

## How to test
tests, compiler

## Related to #2414
@github-actions GitHub Actions
Copy link

This issue has been marked as stale because it has been inactive a while. Please update this issue or it will be automatically closed.

@github-actions github-actions bot added the Stale label Apr 21, 2024
@MartinquaXD
Copy link
Contributor Author

Already fixed; just forgot to close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@MartinquaXD and others