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

Add runtime API ConvertTransactionRuntime API #559

Merged

Conversation

librelois
Copy link
Contributor

@librelois librelois commented Jan 25, 2022

Closes #508

Introduces a new runtime API (ConvertTransactionRuntimeApi) to convert an ethereum transaction to an Extrinsic.

This is necessary for production networks that want to move to eth transactions V2 without a breaking update (like Moonbeam).

The TransactionConverter is still used if the runtime API is not present.

For projects that integrate the runtime API into their runtime, a TransactionConverter must still be provided to satisfy the rust traits bound, which is why a NoTransactionCorverter type was introduced.

@librelois librelois requested a review from sorpaas as a code owner January 25, 2022 13:54

#[api_version(2)]
pub trait ConvertTransactionRuntimeApi {
fn convert_transaction(transaction: ethereum::TransactionV2) -> <Block as BlockT>::Extrinsic;
Copy link
Member

@sorpaas sorpaas Feb 2, 2022

Choose a reason for hiding this comment

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

Consider changing this to with RLP Vec<u8> as input, and return Option<Extrinsic> where None indicates that the conversion failed.

Ethereum transaction's RLP encoding is backward and future compatible, but not really the SCALE encoding, as that's internal. Doing the above ensures that when runtime updates to support a new transaction type, you can immediately use it without waiting for old nodes to update. With this you can also remove the versioning.

Also, is there a particular reason that this isn't put into EthereumRuntimeRPCApi?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I realized that our EthereumRuntimeRPCApi is not forward compatible anyway, so the above (using RLP Vec<u8> as input) may not provide much benefits.

The other points still stands -- maybe consider moving this to EthereumRuntimeRPCApi.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We decided to have it in a separate trait so that the client can ask if the API is present or not.
Indeed, substrate does not allow to ask if a particular method of a runtime api is present or not.


// `NoTransactionConverter` is a non-instantiable type (an enum with no variants),
// so we are guaranteed at compile time that `NoTransactionConverter` can never be instantiated.
pub enum NoTransactionConverter {}
Copy link
Member

@sorpaas sorpaas Feb 2, 2022

Choose a reason for hiding this comment

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

Is this actually used? Do we actually have a situation where transaction conversion is just not possible or something?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you have this type, then why do you still have an Option<_> in Eth::new?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I get it. I use this only as a type parameter. and use None as the value. That's why it's not instantiable.

Copy link
Member

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

I talked with @tgmichel and let's keep it the current way, as the code is already semi-used in production.

@sorpaas sorpaas merged commit d5d4f94 into polkadot-evm:master Feb 2, 2022
@MOZGIII
Copy link
Contributor

MOZGIII commented Feb 4, 2022

I'd like to highlight some differences compared to #558:

  • This version is more verbose, and there's a lot more logic that's copy-pasted around;
  • In the Backport of runtime transaction converter #558, the converter is made to be fallible, which allows to implement it over the client if the Runtime API needed;
  • The fallback logic is hardcoded here, and represented via Option, while in the Backport of runtime transaction converter #558 version it can just be a variant of TransactionConverter implementation - which makes way more sense for networks that start from scratch and don't need this legacy weight - they'd just use the API version from the genesis.

It is very sad to see that #558 didn't got the attention it deserved.

@librelois librelois deleted the elois-convert-tx-runtime-api branch February 4, 2022 18:58
@librelois
Copy link
Contributor Author

I'd like to highlight some differences compared to #558:

* This version is more verbose, and there's a lot more logic that's copy-pasted around;

* In the [Backport of runtime transaction converter #558](https://github.com/paritytech/frontier/pull/558), the converter is made to be fallible, which allows to implement it over the client if the Runtime API needed;

* The fallback logic is hardcoded here, and represented via `Option`, while in the [Backport of runtime transaction converter #558](https://github.com/paritytech/frontier/pull/558) version it can just be a variant of `TransactionConverter` implementation - which makes way more sense for networks that start from scratch and don't need this legacy weight - they'd just use the API version from the genesis.

It is very sad to see that #558 didn't got the attention it deserved.

Sorry @MOZGIII , I hadn't seen your PR, and we had to implement it in a hurry to solve a problem in production :/

Your PR seems to be better designed, you can open a new one to improve the implementation, and tag me to review it :)
My only constraint is that it has to be compatible with what is already in production, so if the runtime API changes you have to upgrade to version 3 :)

abhijeetbhagat pushed a commit to web3labs/frontier that referenced this pull request Jan 11, 2023
* Add runtime API `ConvertTransactionRuntime API`

* ref: make transaction_converter optional

* rustfmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace TransactionConverter with a runtime API?
4 participants