feat(l1): implement the net_version rpc method #1037
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fmoletta
reviewed
Oct 31, 2024
…ring unwrap edge case (lambdaclass#1018) **Motivation** <!-- Why does this pull request exist? What are its goals? --> To avoid panicking when the transaction filter fails to filter a transaction because we do not yet filter transactions with invalid tip. **Description** - Filter transactions whose effective gas tip calculation underflowed. - Handle the unwrap of the `effective_gas_tip` calculation. Even though this is an edge case that shouldn't happen, it is better to have a descriptive error if it happens someday than to panic. --------- Co-authored-by: fmoletta <[email protected]>
**Motivation** <!-- Why does this pull request exist? What are its goals? --> Nowadays, all the commands that send transactions do not wait for transaction receipts. If you run the same command multiple times the same transaction with the same nonce is going to be sent to the node; another problem is that the users have to perform additional steps to make sure that their transaction was finalized or not. As this is not an implementation problem but a misuse of the CLI, it'd be good for users to also have the option to wait for their transactions to be finalized. **Description** Adds a `-w` flag to the cmds `deploy`, `transfer`, `send`, `deposit`, `withdraw`, and `claim-withdraw` which when set, waits for the transaction sent to be finalized (a.k.a. wait for its receipt).
**Motivation** Have the tx spammer running instead of immediatly shutting down when we are the first node in the devnet setup. **Description** This PR tackled a couple of issues until we were able to process an initial transaction, (they are small changes): - We were returning an error on `eth_getTransactionCount` when we either didn't have a `latest` block before genesis or at any point when we don't have `pending` blocks. The solution in this case was returning `0x0` in those cases. - When requesting `eth_getTransactionCount` on `pending` after some transaction went through we were defaulting to `0x0`, it appears that the idea is to default to the `latest` in those case which make sense. - There were a missing filter that made the node panic when building payloads for transactions with fees lower than the base_fee_per_gas, this generated that those kind of transactions stopped the node's ability to build blocks when they were in the mempool, we made a quick workaround in this PR, but will remove it in favor of lambdaclass#1018 Closes lambdaclass#1026
**Motivation** <!-- Why does this pull request exist? What are its goals? --> We currently have an in-memory database that should soon be replaced by the node's in-disk database. For that purpose we want to allow us to switch between both kinds of databases. The need to implement this PR's features and refactor the `Db` arose while working on lambdaclass#904. **Description** <!-- A clear and concise general description of the changes this PR introduces --> This PR includes: - Adding a `Cache` to store warm accounts. This removes the need of having `accessed_accounts` and `accessed_storage_slots` sets in `Substate` because we know that if they are cached then they are warm. - Making our `Db` implement the `Database` trait and interact with it only using methods and not it's attributes, so in the future we can implement that trait for the actual node's database. - Fix call opcodes and remove delegate attribute from `CallFrame`. <!-- Link to issues: Resolves lambdaclass#111, Resolves lambdaclass#222 --> Part of lambdaclass#814. --------- Co-authored-by: Juani Medone <[email protected]> Co-authored-by: maximopalopoli <[email protected]> Co-authored-by: Javier Chatruc <[email protected]>
…andling logic (lambdaclass#960) **Motivation** Handling snap capability message `GetAccountRange` <!-- Why does this pull request exist? What are its goals? --> **Description** * Add functionality to iterate the Trie (TrieIterator) * Add functionality to iterate over all accounts in the state (Store::iter_accounts) * Add logic to handle `GetAccountRange` snap request * Fix slim encoding of `AccountState` * Remove unneeded trait `RLPEncodeSlim` **Notes** * ~We don't have the listen loop implemented so this PR only adds the standalone logic for handling the request and creating a response, we still need to plug it in to the main loop.~ * ~We are not able to run the hive test suite due to missing listen loop + old blocks being used by the test suite. I instead copied the state from a Geth execution (loading genesis + importing chain) and used that state to replicate hive tests as unit tests. These tests could be removed once we fix those two problems~ <!-- A clear and concise general description of the changes this PR introduces --> <!-- Link to issues: Resolves lambdaclass#111, Resolves lambdaclass#222 --> Partially addresses lambdaclass#184 --------- Co-authored-by: Esteban Dimitroff Hodi <[email protected]>
Arkenan
reviewed
Nov 13, 2024
crates/networking/rpc/net/mod.rs
Outdated
| let chain_spec = context | ||
| .storage | ||
| .get_chain_config() | ||
| .map_err(|error| RpcErr::Internal(error.to_string()))?; |
There was a problem hiding this comment.
Do we need this map_err? Can't we just use the question mark like we do in engine/fork_choice.rs:106? We are calling get_chain_config there as well. The store error should correctly propagate as a RpcErr.
This is because the From<StoreError> for RpcErr trait is implemented in rpc/utils.rs
There was a problem hiding this comment.
ah i didn't know that. Will remove the explicit map_err()!
crates/networking/rpc/net/mod.rs
Outdated
| .get_chain_config() | ||
| .map_err(|error| RpcErr::Internal(error.to_string()))?; | ||
| serde_json::to_value(format!("{}", chain_spec.chain_id)) | ||
| .map_err(|error| RpcErr::Internal(error.to_string())) |
There was a problem hiding this comment.
Same here. Can't we just omit the map_err? The From<serde_json::Error> for RpcErr trait is implemented too.
There was a problem hiding this comment.
We can go about in 2 ways:
- a shorter version of map_err:
serde_json::to_value(format!("{}", chain_spec.chain_id))
.map_err(RpcErr::from)
- extract value and wrap it in Ok():
let value = serde_json::to_value(format!("{}", chain_spec.chain_id))?;
Ok(value)
Which is the preferred style señor?
|
Hi @h3lio5 , can you fix the CI plz? |
|
@mpaulucci yes, I ran |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Implement the net_version rpc method
Closes #942