Skip to content
This repository has been archived by the owner on Sep 28, 2023. It is now read-only.

XVM MVP v2 #92

Merged
merged 14 commits into from
Nov 8, 2022
3 changes: 0 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,4 @@ members = [
"precompiles/utils",
"precompiles/xcm",
"precompiles/xvm",

# Disabled due to incorrect nightly channel usage
# "contracts/*",
]
4 changes: 0 additions & 4 deletions chain-extensions/types/xvm/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,15 @@ description = "Types definitions for contracts using xvm chain-extension."

[dependencies]
codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false }
frame-support = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.30", default-features = false }
scale-info = { version = "2.1.0", default-features = false, features = ["derive"] }
sp-core = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.30", default-features = false }
sp-runtime = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.30", default-features = false }
sp-std = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.30", default-features = false }

[features]
default = ["std"]
std = [
"codec/std",
"frame-support/std",
"scale-info/std",
"sp-core/std",
"sp-runtime/std",
"sp-std/std",
]
5 changes: 2 additions & 3 deletions chain-extensions/types/xvm/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
#![cfg_attr(not(feature = "std"), no_std)]

use codec::{Decode, Encode};
use sp_runtime::{DispatchError, ModuleError};
use sp_std::vec::Vec; // TODO use ink_prelude::vec::Vec; ?
use sp_std::vec::Vec;

#[cfg_attr(feature = "std", derive(scale_info::TypeInfo))]
#[derive(PartialEq, Eq, Copy, Clone, Encode, Decode, Debug)]
Expand Down Expand Up @@ -35,8 +36,6 @@ pub struct XvmCallArgs {
pub to: Vec<u8>,
/// Encoded call params
pub input: Vec<u8>,
/// Metadata for the encoded params
pub metadata: Vec<u8>,
}

pub const FRONTIER_VM_ID: u8 = 0x0F;
Expand Down
33 changes: 14 additions & 19 deletions chain-extensions/xvm/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
#![cfg_attr(not(feature = "std"), no_std)]

use frame_support::weights::Weight;
use frame_system::RawOrigin;
use pallet_contracts::chain_extension::{
ChainExtension, Environment, Ext, InitState, RetVal, SysConfig, UncheckedFrom,
Expand Down Expand Up @@ -52,27 +51,19 @@ where

match func_id {
XvmFuncId::XvmCall => {
// 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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

let remaining_weight = env.ext().gas_meter().gas_left();
let charged_weight = env.charge_weight(remaining_weight)?;

// Prepare parameters
let caller = env.ext().caller().clone();

let XvmCallArgs {
vm_id,
to,
input,
metadata,
} = env.read_as_unbounded(env.in_len())?;
let XvmCallArgs { vm_id, to, input } = env.read_as_unbounded(env.in_len())?;

// 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();
Copy link
Member

Choose a reason for hiding this comment

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

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

let _value = env.ext().value_transferred();
let _gas_limit = env.ext().gas_meter().gas_left();
let xvm_context = XvmContext {
id: T::VmId::from(vm_id),
id: vm_id,
max_weight: remaining_weight,
env: None,
};

Expand All @@ -81,12 +72,16 @@ where
xvm_context,
to,
input,
metadata,
);

// TODO: We need to know how much of gas was spent in the other call and update the gas meter!
// let consumed_xvm_weight = ...;
// env.charge_weight(consumed_xvm_weight)?;
// Adjust the actual weight used by the call if needed.
let actual_weight = match call_result {
Ok(e) => e.actual_weight,
Err(e) => e.post_info.actual_weight,
};
if let Some(actual_weight) = actual_weight {
env.adjust_weight(charged_weight, actual_weight);
}

return match call_result {
Err(e) => {
Expand Down
42 changes: 0 additions & 42 deletions contracts/xvm/Cargo.toml

This file was deleted.

85 changes: 0 additions & 85 deletions contracts/xvm/src/lib.rs

This file was deleted.

60 changes: 45 additions & 15 deletions frame/collator-selection/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it’s new astar-frame formatting settings

RuntimeOrigin::signed(4)
));

// but no more
assert_noop!(
Expand All @@ -132,7 +134,9 @@ fn cannot_unregister_candidate_if_too_few() {
new_test_ext().execute_with(|| {
// reset desired candidates:
<crate::DesiredCandidates<Test>>::put(1);
assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4)));
assert_ok!(CollatorSelection::register_as_candidate(
RuntimeOrigin::signed(4)
));

// can not remove too few
assert_noop!(
Expand Down Expand Up @@ -170,7 +174,9 @@ fn cannot_register_as_candidate_if_keys_not_registered() {
fn cannot_register_dupe_candidate() {
new_test_ext().execute_with(|| {
// can add 3 as candidate
assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3)));
assert_ok!(CollatorSelection::register_as_candidate(
RuntimeOrigin::signed(3)
));
let addition = CandidateInfo {
who: 3,
deposit: 10,
Expand All @@ -194,7 +200,9 @@ fn cannot_register_as_candidate_if_poor() {
assert_eq!(Balances::free_balance(&33), 0);

// works
assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3)));
assert_ok!(CollatorSelection::register_as_candidate(
RuntimeOrigin::signed(3)
));

// poor
assert_noop!(
Expand All @@ -217,8 +225,12 @@ fn register_as_candidate_works() {
assert_eq!(Balances::free_balance(&3), 100);
assert_eq!(Balances::free_balance(&4), 100);

assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3)));
assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4)));
assert_ok!(CollatorSelection::register_as_candidate(
RuntimeOrigin::signed(3)
));
assert_ok!(CollatorSelection::register_as_candidate(
RuntimeOrigin::signed(4)
));

assert_eq!(Balances::free_balance(&3), 90);
assert_eq!(Balances::free_balance(&4), 90);
Expand All @@ -231,11 +243,15 @@ fn register_as_candidate_works() {
fn leave_intent() {
new_test_ext().execute_with(|| {
// register a candidate.
assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3)));
assert_ok!(CollatorSelection::register_as_candidate(
RuntimeOrigin::signed(3)
));
assert_eq!(Balances::free_balance(3), 90);

// register too so can leave above min candidates
assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5)));
assert_ok!(CollatorSelection::register_as_candidate(
RuntimeOrigin::signed(5)
));
assert_eq!(Balances::free_balance(5), 90);

// cannot leave if not candidate.
Expand All @@ -259,7 +275,9 @@ fn authorship_event_handler() {

// 4 is the default author.
assert_eq!(Balances::free_balance(4), 100);
assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4)));
assert_ok!(CollatorSelection::register_as_candidate(
RuntimeOrigin::signed(4)
));
// triggers `note_author`
Authorship::on_initialize(1);

Expand Down Expand Up @@ -287,7 +305,9 @@ fn fees_edgecases() {
Balances::make_free_balance_be(&CollatorSelection::account_id(), 5);
// 4 is the default author.
assert_eq!(Balances::free_balance(4), 100);
assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4)));
assert_ok!(CollatorSelection::register_as_candidate(
RuntimeOrigin::signed(4)
));
// triggers `note_author`
Authorship::on_initialize(1);

Expand Down Expand Up @@ -319,7 +339,9 @@ fn session_management_works() {
assert_eq!(SessionHandlerCollators::get(), vec![1, 2]);

// add a new collator
assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3)));
assert_ok!(CollatorSelection::register_as_candidate(
RuntimeOrigin::signed(3)
));

// session won't see this.
assert_eq!(SessionHandlerCollators::get(), vec![1, 2]);
Expand Down Expand Up @@ -348,8 +370,12 @@ fn kick_and_slash_mechanism() {
// Define slash destination account
<crate::SlashDestination<Test>>::put(5);
// add a new collator
assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3)));
assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4)));
assert_ok!(CollatorSelection::register_as_candidate(
RuntimeOrigin::signed(3)
));
assert_ok!(CollatorSelection::register_as_candidate(
RuntimeOrigin::signed(4)
));
initialize_to_block(10);
assert_eq!(CollatorSelection::candidates().len(), 2);
initialize_to_block(20);
Expand Down Expand Up @@ -377,8 +403,12 @@ fn kick_and_slash_mechanism() {
fn should_not_kick_mechanism_too_few() {
new_test_ext().execute_with(|| {
// add a new collator
assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3)));
assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5)));
assert_ok!(CollatorSelection::register_as_candidate(
RuntimeOrigin::signed(3)
));
assert_ok!(CollatorSelection::register_as_candidate(
RuntimeOrigin::signed(5)
));
initialize_to_block(10);
assert_eq!(CollatorSelection::candidates().len(), 2);
initialize_to_block(20);
Expand Down
Loading