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

[pallet-revive] Add Ethereum JSON-RPC server #6147

Open
wants to merge 193 commits into
base: master
Choose a base branch
from

Conversation

pgherveou
Copy link
Contributor

@pgherveou pgherveou commented Oct 21, 2024

Redo of #5953

@pgherveou pgherveou changed the base branch from master to pg/fix-hardcoded-gas October 24, 2024 06:48
Base automatically changed from pg/fix-hardcoded-gas to master October 24, 2024 11:03
Copy link
Member

@xermicus xermicus left a comment

Choose a reason for hiding this comment

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

Very nice! Definitively gonna fire up some of the examples to try it out

substrate/bin/node/runtime/Cargo.toml Outdated Show resolved Hide resolved
substrate/frame/revive/Cargo.toml Outdated Show resolved Hide resolved
@@ -48,7 +48,7 @@ type CallOf<T> = <T as frame_system::Config>::RuntimeCall;
/// We use a fixed value for the gas price.
/// This let us calculate the gas estimate for a transaction with the formula:
/// `estimate_gas = substrate_fee / gas_price`.
pub const GAS_PRICE: u32 = 1_000u32;
pub const GAS_PRICE: u32 = 1u32;
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional? Might also be worthwhile to explain the bases of the estimate in the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think either value is fine since they are constant

1 is nice since we can compute tx.gas = dry_run.fee / GAS_PRICE, there are no rest to the division

prdoc/pr_6192.prdoc Outdated Show resolved Hide resolved
#[async_trait]
impl MiscRpcServer for MiscRpcServerImpl {
async fn healthcheck(&self) -> RpcResult<()> {
Ok(())
Copy link
Member

Choose a reason for hiding this comment

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

Is the idea here just to be able to ping the server?

Wondering why this is a trait, seems like if this is just to have a ping like check it seems superfluous to have a trait for it.

Copy link
Contributor Author

@pgherveou pgherveou Oct 24, 2024

Choose a reason for hiding this comment

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

Is the idea here just to be able to ping the server?

Yes, a trait for every other rpc.
I can probably remove this one for now,

with jsonrpsee crate you define your rpc on a trait with the #[rpc] macro and then implement it on the server, the client trait is generated as well for you through this Macro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T7-smart_contracts This PR/Issue is related to smart contracts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants