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

Migrate from self-contained call to new General Extrinsic Format #1633

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

Conversation

vedhavyas
Copy link
Contributor

This PR migrate old Self-Contained Bare extrinsic format to new General Extrinsic format.

Notable changes

  • Transaction validation checks are moved to newly defined Extension
  • Weight check is removed from pallet-ethereum but it is implicitly checked through Extensions defined.
  • Removed self-contained crate as this wont be required anymore

For runtimes that have impl extra logic in their runtime, I have provided a new trait that will be impl by the RuntimeCall

pub trait EthereumTransactionHook<Runtime>
where
	Runtime: Config,
	OriginFor<Runtime>: Into<Result<RawOrigin, OriginFor<Runtime>>>,
{
	fn maybe_ethereum_call(&self) -> Option<&EthereumCall<Runtime>>;

	fn additional_validation(
		&self,
		_signer: H160,
		_source: TransactionSource,
	) -> Result<(), TransactionValidityError> {
		Ok(())
	}
}

additional_validation will be called with signer and the source for any custom validation apart from the common validations pallet-ethereum provides.

@vedhavyas
Copy link
Contributor Author

vedhavyas commented Mar 19, 2025

Two integration tests are failing

Frontier RPC (Gas limit Weightv2 ref time)
This test should ideally include all the 130 transactions but now it only includes 123
Also noticed, gasUsed is 73117862 which gives a diff of 1882138 remaining gas.

Any suggestions on why remaining 7 txns were not included in the block would be greatly appreciated

I believe the same is the case with Frontier RPC (Gas limit Weightv2 pov size) test as well

update:

If I create a new block, then rest of the transactions are included.
@sorpaas @conr2d if you have any insights I would appreciate to get this going forward

@vedhavyas vedhavyas force-pushed the general_extrinsics branch from 850e356 to cd91c32 Compare March 19, 2025 14:01
@vedhavyas vedhavyas force-pushed the general_extrinsics branch from cd91c32 to 31938d3 Compare March 19, 2025 14:02
@conr2d
Copy link
Contributor

conr2d commented Mar 20, 2025

I'm a bit busy these days, so I'll leave a review soon. However, I don't have merge rights, so my review doesn't guarantee merging this PR.

One immediate comment: fp-self-contained shouldn't be removed at this stage. This removal would break the build for many downstream projets using Frontier. For now, it's better to add a #[deprecated] attribute to fp_self_contained::UncheckedExtrinsic and implement the new approach in the runtime template as you did.

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.

2 participants