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] update generic runtime types #5608

Merged
merged 22 commits into from
Sep 8, 2024

Conversation

pgherveou
Copy link
Contributor

@pgherveou pgherveou commented Sep 5, 2024

fix #5574

  • Use U256 instead of BalanceOf and MomentOf in Ext trait
  • Enforce H256 for T::Hash

The Ext trait still depends on the associated type T: Config, we can look into refactoring it even more later but even in the current state it should not influence how the data is encoded / decoded between the contract and the host

fn caller(&self) -> Origin<Self::T>;
-> only use to extract the address of the caller 

fn account_id(&self) -> &AccountIdOf<Self::T>;
 -> only used to expose the address or access the account_id internally   

fn gas_meter(&self) -> &GasMeter<Self::T>;
fn gas_meter_mut(&mut self) -> &mut GasMeter<Self::T>;
 -> encoding does not depend on T

fn call_runtime(&self, call: <Self::T as Config>::RuntimeCall) -> DispatchResultWithPostInfo;
-> Substrate specific, just an opaque blob of bytes from the contract's perspective

fn contract_info(&mut self) -> &mut ContractInfo<Self::T>;
fn transient_storage(&mut self) -> &mut TransientStorage<Self::T>;
-> gated by #[cfg(any(test, feature = "runtime-benchmarks"))]

@pgherveou pgherveou added the T7-smart_contracts This PR/Issue is related to smart contracts. label Sep 5, 2024
@pgherveou pgherveou marked this pull request as ready for review September 5, 2024 20:34
@athei athei added R0-silent Changes should not be mentioned in any release notes and removed T7-smart_contracts This PR/Issue is related to smart contracts. labels Sep 6, 2024
@athei
Copy link
Member

athei commented Sep 6, 2024

Let's stay silent before 1.0 to not clutter the changelog with information of a not ready to use pallet. Still need a prdoc for the bumps though.

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.

LGTM

Tested your branch with the compiler, the balance syscall passes against geth EVM for selfbalance

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Can you remove the usage of SCALE where it became possible due to having a concrete type? I put some example comments where this was the case.

And yeah let's not sweat removing the T for now.

substrate/frame/revive/src/wasm/runtime.rs Outdated Show resolved Hide resolved
substrate/frame/revive/src/wasm/runtime.rs Outdated Show resolved Hide resolved
substrate/frame/revive/src/wasm/runtime.rs Outdated Show resolved Hide resolved
substrate/frame/revive/src/wasm/runtime.rs Outdated Show resolved Hide resolved
@pgherveou
Copy link
Contributor Author

Can you remove the usage of SCALE where it became possible due to having a concrete type? I put some example comments where this was the case.

And yeah let's not sweat removing the T for now.

Made the change in the last commit. SCALE is mostly used now for substrate specific host fns (call_runtime and xcm)
1b2ed74

@athei athei enabled auto-merge September 8, 2024 19:47
@athei athei added this pull request to the merge queue Sep 8, 2024
github-merge-queue bot pushed a commit that referenced this pull request Sep 8, 2024
fix #5574

- Use U256 instead of BalanceOf<T> and MomentOf<T> in Ext trait
- Enforce H256 for T::Hash

The Ext trait still depends on the associated type `T: Config`, we can
look into refactoring it even more later but even in the current state
it should not influence how the data is encoded / decoded between the
contract and the host
```
fn caller(&self) -> Origin<Self::T>;
-> only use to extract the address of the caller 

fn account_id(&self) -> &AccountIdOf<Self::T>;
 -> only used to expose the address or access the account_id internally   

fn gas_meter(&self) -> &GasMeter<Self::T>;
fn gas_meter_mut(&mut self) -> &mut GasMeter<Self::T>;
 -> encoding does not depend on T

fn call_runtime(&self, call: <Self::T as Config>::RuntimeCall) -> DispatchResultWithPostInfo;
-> Substrate specific, just an opaque blob of bytes from the contract's perspective

fn contract_info(&mut self) -> &mut ContractInfo<Self::T>;
fn transient_storage(&mut self) -> &mut TransientStorage<Self::T>;
-> gated by #[cfg(any(test, feature = "runtime-benchmarks"))]
```
@athei athei removed this pull request from the merge queue due to a manual request Sep 8, 2024
@athei athei enabled auto-merge September 8, 2024 19:50
@athei athei added this pull request to the merge queue Sep 8, 2024
Merged via the queue into master with commit 868a36b Sep 8, 2024
205 of 206 checks passed
@athei athei deleted the pg/revive_more_types_changed branch September 8, 2024 21:56
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
Projects
None yet
3 participants