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

Pallet-scheduler Chain extension #148

Draft
wants to merge 2 commits into
base: polkadot-v0.9.39
Choose a base branch
from

Conversation

PierreOssun
Copy link
Member

Pull Request Summary
Added two calls for pallet-scheduler chain extension:
Schedule
It schedule a call to another smart contract. It can be periodic or not. Origin is set as contract address

Cancel
The caller can cancel its call (only smart-contracts can be caller)

Also added tests for it.

Check list

  • added unit tests

@github-actions
Copy link

Code Coverage

Package Line Rate Branch Rate Health
primitives/xcm/src 62% 0%
chain-extensions/types/dapps-staking/src 0% 0%
frame/pallet-xcm/src 53% 0%
frame/xc-asset-config/src 50% 0%
precompiles/utils/macro/tests 0% 0%
frame/custom-signatures/src 52% 0%
frame/dapps-staking/src/pallet 87% 0%
precompiles/assets-erc20/src 76% 0%
chain-extensions/types/scheduler/src 0% 0%
precompiles/sr25519/src 79% 0%
chain-extensions/pallet-assets/src 0% 0%
chain-extensions/xvm/src 0% 0%
precompiles/utils/macro/src 0% 0%
precompiles/xvm/src 61% 0%
chain-extensions/pallet-scheduler/src 57% 0%
frame/block-reward/src 80% 0%
precompiles/dapps-staking/src 93% 0%
frame/pallet-xvm/src/pallet 8% 0%
chain-extensions/types/assets/src 0% 0%
precompiles/substrate-ecdsa/src 78% 0%
frame/collator-selection/src 76% 0%
frame/contracts-migration/src 0% 0%
chain-extensions/dapps-staking/src 0% 0%
chain-extensions/types/xvm/src 0% 0%
frame/dapps-staking/src 82% 0%
frame/pallet-xvm/src 5% 0%
precompiles/utils/src 69% 0%
precompiles/xcm/src 85% 0%
Summary 56% (2716 / 4826) 0% (0 / 0)

Minimum allowed line rate is 50%

Comment on lines +69 to +75
pub struct SchedulerExtension<T, W>(PhantomData<(T, W)>);

impl<T, W> Default for SchedulerExtension<T, W> {
fn default() -> Self {
SchedulerExtension(PhantomData)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub struct SchedulerExtension<T, W>(PhantomData<(T, W)>);
impl<T, W> Default for SchedulerExtension<T, W> {
fn default() -> Self {
SchedulerExtension(PhantomData)
}
}
#[derive(DefaultNoBound)]
pub struct SchedulerExtension<T, W>(PhantomData<(T, W)>);

No need to manually impl Default, use use frame_support::DefaultNoBound;

Comment on lines +45 to +62
enum SchedulerFunc {
Schedule,
Cancel,
}

impl TryFrom<u16> for SchedulerFunc {
type Error = DispatchError;

fn try_from(value: u16) -> Result<Self, Self::Error> {
match value {
1 => Ok(SchedulerFunc::Schedule),
2 => Ok(SchedulerFunc::Cancel),
_ => Err(DispatchError::Other(
"PalletSchedulerExtension: Unimplemented func_id",
)),
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enum SchedulerFunc {
Schedule,
Cancel,
}
impl TryFrom<u16> for SchedulerFunc {
type Error = DispatchError;
fn try_from(value: u16) -> Result<Self, Self::Error> {
match value {
1 => Ok(SchedulerFunc::Schedule),
2 => Ok(SchedulerFunc::Cancel),
_ => Err(DispatchError::Other(
"PalletSchedulerExtension: Unimplemented func_id",
)),
}
}
}
#[repr(u16)]
#[derive(TryFromPrimitive)]
enum SchedulerFunc {
Schedule = 1,
Cancel = 2,
}

Using num_enum will make this less error prone, right now enum values are not mapping 1 to 1 with TryFrom implementation which is not ideal.

Comment on lines +74 to +85
#[derive(Debug, Copy, Clone, PartialEq, Eq, Encode, Decode, MaxEncodedLen)]
#[cfg_attr(feature = "std", derive(scale_info::TypeInfo))]
pub enum Origin {
Caller,
Address,
}

impl Default for Origin {
fn default() -> Self {
Self::Address
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[derive(Debug, Copy, Clone, PartialEq, Eq, Encode, Decode, MaxEncodedLen)]
#[cfg_attr(feature = "std", derive(scale_info::TypeInfo))]
pub enum Origin {
Caller,
Address,
}
impl Default for Origin {
fn default() -> Self {
Self::Address
}
}
#[derive(Default, Debug, Copy, Clone, PartialEq, Eq, Encode, Decode, MaxEncodedLen)]
#[cfg_attr(feature = "std", derive(scale_info::TypeInfo))]
pub enum Origin {
Caller,
#[default]
Address,
}

@ashutoshvarma
Copy link
Member

ashutoshvarma commented May 1, 2023

A General comment on the CE code structure we follow currently.

We have types crate for each CE to allow importing CE types without depending on CE itself (at least I believe that, correct me if wrong). That is useful for ink! contracts to build the CE call as they required the input, output and error types that are being used in CE.

If above assumption is correct then why is there a need to copy past the types to contract helpers in chain-extension-contract repo - https://github.com/swanky-dapps/chain-extension-contracts/blob/ad0787b7e0e087493efaf00610a027356603a0a9/crates/scheduler/lib.rs#L44-L112?
Why not simply add types as dep to contract and import it directly? Maybe I'm missing something?

@PierreOssun
Copy link
Member Author

Because of the features hell that it will require:
Check this discussion

@ashutoshvarma
Copy link
Member

I understand, we don't want mutually exclusive features to make sure cargo check --all-features always work.

From my understanding, in the types crate we only need ink_env dependency because we need to implement FromStatusCode trait for the Error/Outcome type. Since clearly, we cannot have ink_env as dep in types (it depends on ink_allocator which cannot be built in std) so FromStatusCode then needs to be implemented in contract side which we cannot as implementing external traits for external types in not allowed in rust.

We have few options to fix this,

  1. Implement a decl macro which will generate the Outcome/Error enum. This can then be used in contract side to generate the same enum.

  2. Wrap the Outcome/Error in struct in contract side and implement the FromStatusCode on it. Since scale encoding for wrapper is exactly the same to the inner types, we don't have to worry about serialization clash.

  3. Open issue in substrate to move ink_env::chain_extension::FromStatusCode to ink_primitives::chain_extension::FromStatusCode

@ashutoshvarma
Copy link
Member

Here is the example for 1st approach, (code from XCM CE)
The primitives crate has all the shared types which both pallet/CE and ink! contract can share.
This is the macro to generate CE Error type - https://github.com/AstarNetwork/Astar/blob/8441e0f3715f2389c8754c74a6a50500d204525c/xcm/pallet-xcm-transactor/primitives/src/lib.rs#L72-L99
and it is used like this in ink! contract - https://github.com/AstarNetwork/Astar/blob/8441e0f3715f2389c8754c74a6a50500d204525c/xcm/pallet-xcm-transactor/contracts-example/sdk.rs#L9-L18

@PierreOssun PierreOssun marked this pull request as draft May 2, 2023 09:22
@PierreOssun
Copy link
Member Author

@ashutoshvarma @Dinonard I think we can close this

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

Successfully merging this pull request may close these issues.

None yet

2 participants