XVM MVP v2 #92

akru · 2022-09-07T06:23:38Z

Pull Request Summary

This PR containts XVM v2 prototype.

Feature list:

Contracts arguments encoding;
Error handling;
Return data handling;
XVM runtime interface & RPC.

Check list

added unit tests
updated documentation

* Init commit * Small fixes * Chain extension & precompile modifications * TODO handling * Traces, improved gas tracking, fixed issues * Doc to placeholder weight * Doc & typos * Change base * Fix UT * Update TODOs, cleanup * Fix contract issues

* Remove chain-extension-trait package * Dapps Staking CE optimizations * Dapps Staking CE optimizations * Rename dapps-staking CE in general way * Derive Default for chain extension structs * Implement Default trait for CEs * Fix typo

…vm-pallet-v2

Dinonard · 2022-11-02T07:49:44Z

chain-extensions/xvm/src/lib.rs

-                // TODO: correct weight calculation directly from pallet!
-                let weight = Weight::from_ref_time(1_000_000_000);
-                env.charge_weight(weight)?;
+                // We need to immediately charge for the worst case scenario. Gas equals Weight in pallet-contracts context.


I was confused at first by this because it seems so familiar lol 😅

I believe gas charging should be rewritten for new weighs, including comments. Let’s do it in XVMv3

I meant to say that the reason this code looked so familiar is that I was the author. In the PR that added weight handling. xD

But it was like 2 months ago so I've forgotten about it.

This handling should be fine - some stuff needs to be benchmarked but that won't change the current weights too much.

Dinonard · 2022-11-02T07:51:29Z

frame/collator-selection/src/tests.rs

@@ -117,7 +117,9 @@ fn cannot_register_candidate_if_too_many() {

        // reset desired candidates:
        <crate::DesiredCandidates<Test>>::put(1);
-        assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4)));
+        assert_ok!(CollatorSelection::register_as_candidate(


Why are unrelated files changed by the formatter? Are you using different settings?

I guess it’s new astar-frame formatting settings

Dinonard · 2022-11-02T07:53:30Z

frame/pallet-xvm/src/evm.rs

+            error: XvmError::ExecutionError(Vec::default()), // TODO: make error mapping make more sense
+            consumed_weight: 42u64, // TODO: res.actual_weight.map(|x| x.ref_time()).unwrap_or(context.max_weight),


We should go with worst case scenario here and specify consumed_weight: context.max_weight

Yes, but it not works with new weighs, so, I just hack it to make it works.

Can you please explain more? What doesn't work?

I believe problem here is different types for Weight and context types. We should rework it using new substrate weight system.

Dinonard · 2022-11-02T07:54:08Z

frame/pallet-xvm/src/evm.rs

-        Ok(Default::default())
+        Ok(XvmCallOk {
+            output: Default::default(), // TODO: Fill output vec with response from the call
+            consumed_weight: 42u64, // TODO: res.actual_weight.map(|x| x.ref_time()).unwrap_or(context.max_weight),


You should extract correct consumed_weight from the res.

Yes, but next time

Why next time?
Weights should be handled properly in v2, right?

And it's a very minor fix. I guess same question as above, but why is the "correct" code commented out?

Unfortunately this code no more works correctly, it assumes that weight is a number, but it is struct now.
I just want merge this PR because it takes so long time and proper weighting isn’t the main aim of this PR.

I see - however, the struct that was introduced was simply a wrapper around the old u64 value.
That was the initial step, the next one being adding the PoV size.

For this particular case, in the PR, all you need to do is this:

consumed_weight: res.actual_weight.map(|x| x.ref_time()).unwrap_or(context.max_weight.ref_time())

Just add .ref_time() to the max_weight.

Not that simple but fixed

Dinonard · 2022-11-02T07:58:00Z

frame/pallet-xvm/src/wasm.rs

+            error: XvmError::ExecutionError(Vec::default()), // TODO: make error mapping make more sense
+            consumed_weight: 42u64, //TODO: e.post_info.actual_weight.ref_time().unwrap_or(gas_limit),


Same comment as for EVM counter-part, we should use context.max_weight here.

Dinonard · 2022-11-02T07:58:27Z

frame/pallet-xvm/src/wasm.rs

+        Ok(XvmCallOk {
+            output: Default::default(), // TODO: Fill in with output from the call
+            consumed_weight: 42u64, //TODO: e.post_info.actual_weight.ref_time().unwrap_or(gas_limit),


Correct consumed weight can be extracted from the res.

PierreOssun · 2022-11-03T08:33:12Z

chain-extensions/xvm/src/lib.rs


-                // TODO: rethink this? How do we get valid env in chain extension? Need to know which data to encode.
-                // gas limit, taking into account used gas so far?
                let _origin_address = env.ext().address().clone();


I think you can delete these 2 unused values.
_origin_address will only be used in a case of an enum where contract can chose to use caller() or address()
_value will be used only when calling payable function will work

contracts/xvm/Cargo.toml

github-actions · 2022-11-07T03:13:21Z

Package	Line Rate	Branch Rate	Health
precompiles/assets-erc20/src	73%	0%	➖
frame/collator-selection/src	80%	0%	✔
precompiles/utils/src/data	72%	0%	➖
chain-extensions/types/dapps-staking/src	0%	0%	❌
frame/block-reward/src	85%	0%	✔
frame/dapps-staking/src	83%	0%	✔
frame/dapps-staking/src/pallet	90%	0%	✔
chain-extensions/dapps-staking/src	0%	0%	❌
precompiles/xvm/src	94%	0%	✔
frame/xc-asset-config/src	70%	0%	➖
primitives/xcm/src	68%	0%	➖
precompiles/xcm/src	84%	0%	✔
precompiles/substrate-ecdsa/src	78%	0%	➖
chain-extensions/rmrk/src	0%	0%	❌
frame/pallet-xvm/src/pallet	34%	0%	❌
frame/custom-signatures/src	57%	0%	➖
frame/pallet-xvm/src	11%	0%	❌
precompiles/utils/src	72%	0%	➖
chain-extensions/types/rmrk/src	0%	0%	❌
chain-extensions/xvm/src	0%	0%	❌
precompiles/utils/macro/src	0%	0%	❌
frame/pallet-xcm/src	65%	0%	➖
chain-extensions/types/xvm/src	0%	0%	❌
precompiles/sr25519/src	79%	0%	➖
precompiles/dapps-staking/src	93%	0%	✔
precompiles/utils/macro/tests	0%	0%	❌
Summary	60% (2577 / 4328)	0% (0 / 0)	➖

Minimum allowed line rate is 50%

Dinonard

LGTM!

akru mentioned this pull request Sep 22, 2022

Enable pallet XVM prototype 2 AstarNetwork/Astar#751

Merged

5 tasks

akru changed the base branch from polkadot-v0.9.28 to polkadot-v0.9.29 September 23, 2022 03:31

akru added 4 commits October 25, 2022 07:32

XVM interface refactoring

cc48aef

Switch precompiles & chain-extension to XVMv2

8451ee7

XVM gas handling prototype (#94)

5340817

* Init commit * Small fixes * Chain extension & precompile modifications * TODO handling * Traces, improved gas tracking, fixed issues * Doc to placeholder weight * Doc & typos * Change base * Fix UT * Update TODOs, cleanup * Fix contract issues

Remove chain-extension-trait package (#109)

902fcbf

* Remove chain-extension-trait package * Dapps Staking CE optimizations * Dapps Staking CE optimizations * Rename dapps-staking CE in general way * Derive Default for chain extension structs * Implement Default trait for CEs * Fix typo

akru force-pushed the feature/xvm-pallet-v2 branch from 4d6aefe to 902fcbf Compare October 25, 2022 04:34

XVM types cargo deps fix

957f0e6

akru force-pushed the feature/xvm-pallet-v2 branch from 5d72e25 to 957f0e6 Compare October 25, 2022 06:11

akru added 2 commits October 25, 2022 09:17

Build fixes

c2d862f

Compilation fixes

f8e19ed

akru requested review from PierreOssun, Dinonard and 0x7CFE October 25, 2022 06:56

akru marked this pull request as ready for review October 25, 2022 06:56

Fix formatting

2124931

akru changed the base branch from polkadot-v0.9.29 to polkadot-v0.9.30 November 2, 2022 02:53

akru added 5 commits November 2, 2022 05:56

Merge remote-tracking branch 'origin/polkadot-v0.9.30' into feature/x…

8483b6d

…vm-pallet-v2

Fix formatting

649faa1

Fix compilation issues

e0a7ab9

Fix cargo fmt

f88d719

Fix clippy errors

72f8d69

Dinonard suggested changes Nov 2, 2022

View reviewed changes

PierreOssun reviewed Nov 3, 2022

View reviewed changes

Fix weight estimation

f1cbe67

Dinonard approved these changes Nov 8, 2022

View reviewed changes

akru merged commit 772d63c into polkadot-v0.9.30 Nov 8, 2022

akru deleted the feature/xvm-pallet-v2 branch November 8, 2022 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XVM MVP v2 #92

XVM MVP v2 #92

akru commented Sep 7, 2022 •

edited

Loading

Dinonard Nov 2, 2022

akru Nov 3, 2022

Dinonard Nov 3, 2022

akru Nov 7, 2022

Dinonard Nov 2, 2022

akru Nov 3, 2022

Dinonard Nov 2, 2022

akru Nov 3, 2022

Dinonard Nov 3, 2022

akru Nov 4, 2022

Dinonard Nov 2, 2022

akru Nov 3, 2022

Dinonard Nov 3, 2022

akru Nov 4, 2022

Dinonard Nov 4, 2022

akru Nov 7, 2022

Dinonard Nov 2, 2022

akru Nov 7, 2022

Dinonard Nov 2, 2022

akru Nov 3, 2022

akru Nov 7, 2022

PierreOssun Nov 3, 2022

github-actions bot commented Nov 7, 2022

Dinonard left a comment

		error: XvmError::ExecutionError(Vec::default()), // TODO: make error mapping make more sense
		consumed_weight: 42u64, // TODO: res.actual_weight.map(\|x\| x.ref_time()).unwrap_or(context.max_weight),

		error: XvmError::ExecutionError(Vec::default()), // TODO: make error mapping make more sense
		consumed_weight: 42u64, //TODO: e.post_info.actual_weight.ref_time().unwrap_or(gas_limit),

XVM MVP v2 #92

XVM MVP v2 #92

Conversation

akru commented Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 7, 2022

Dinonard left a comment

Choose a reason for hiding this comment

akru commented Sep 7, 2022 •

edited

Loading