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.

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

feat: implement eth_call and refactor trin-execution to support async db #1415

Closed
wants to merge 1 commit into from

Conversation

morph-dev
Copy link
Collaborator

What was wrong?

The eth_call wasn't supported.

How was it fixed?

Refactored logic in trin-execution crate to make functionality more reusable (and added support for async database).
Also refactored rpc/src/eth_rpc.rs by extracting state related functionality into separate file.

To-Do

@morph-dev morph-dev self-assigned this Sep 3, 2024
@morph-dev morph-dev added the state network Issue related to portal state network label Sep 3, 2024
@morph-dev morph-dev added this to the Devcon 2024 milestone Sep 3, 2024
@morph-dev
Copy link
Collaborator Author

@KolbyML , would like your early feedback on this PR, especially on the work in trin-execution.

Note: I didn't test if this actually works, as I don't have data in the state network to test it with. I will have to manually create and seed data needed but it's not simple as I have to seed many trie nodes, for multiple accounts and storage slots (and at least one contract bytecode). Other than testing, I would say that this PR is mostly ready as is.

Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You asked for some early feedback, some it it looks fine. My main concern is this PR cements and creates abstractions for bad practices, this PR is based off latest which doesn't contain the improvements from #1337 yet.

This PR builds off of master which still calls EvmBuilder for every transaction which isn't idle. So my main feedback is to build off #1337

Comment on lines +18 to +38
/// Executes the transaction with external context in the blocking manner.
pub fn execute_transaction_with_external_context<
DB: DatabaseRef,
EXT: GetInspector<WrapDatabaseRef<DB>>,
>(
tx: &impl TxEnvModifier,
evm_environment: Env,
external_context: EXT,
database: DB,
) -> EVMResult<DB::Error> {
let block_number = evm_environment.block.number.to::<u64>();
Evm::builder()
.with_ref_db(database)
.with_env(Box::new(evm_environment))
.with_spec_id(get_spec_id(block_number))
.modify_tx_env(|tx_env| tx.modify(block_number, tx_env))
.with_external_context(external_context)
.append_handler_register(inspector_handle_register)
.build()
.transact()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realistically we wouldn't call Evm::builder() on every transaction execution. This is changed in #1337

I think this PR #1415 should be based off #1337 and #1337 should be merged first*

@morph-dev
Copy link
Collaborator Author

Closing in favor of #1439

@morph-dev morph-dev closed this Sep 11, 2024
@morph-dev morph-dev deleted the eth_call branch September 12, 2024 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state network Issue related to portal state network
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants