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

Poc extension system #27

Merged
merged 13 commits into from
Jun 6, 2024
Merged

Poc extension system #27

merged 13 commits into from
Jun 6, 2024

Conversation

indirection42
Copy link
Contributor

@indirection42 indirection42 commented May 30, 2024

Close #8

@indirection42 indirection42 requested a review from xlc May 30, 2024 00:56
poc/extensions/src/extension_core.rs Outdated Show resolved Hide resolved
poc/extensions/src/lib.rs Outdated Show resolved Hide resolved
@indirection42 indirection42 requested a review from xlc May 31, 2024 08:16
Copy link
Member

@xlc xlc left a comment

Choose a reason for hiding this comment

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

looks mostly fine

poc/extensions/src/dispatchable.rs Outdated Show resolved Hide resolved
type MethodName;
type MethodIndex;
fn query_method(method_name: Self::MethodName) -> Self::MethodIndex;
fn dispatch(self) -> Vec<u8>;
Copy link
Member

Choose a reason for hiding this comment

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

it could be useful to make this return an Result

use super::ExtensionTypeId;
use parity_scale_codec::{Decode, Encode};
// SDK codes
pub trait ExtensionCore: Dispatchable + TryFrom<(u32, Vec<u8>)> {
Copy link
Member

Choose a reason for hiding this comment

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

TryFrom<(u32, Vec<u8>) is not correct. It should be something like TryFrom<(Dispatchable::MethodIndex, Dispatchable::MethodName)>
(you need to figure out the right syntax. maybe with a whare clause(

Copy link
Member

Choose a reason for hiding this comment

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

and should have a trait Extension: Dispatchable + TryFrom<X, Y> and have other extensions conforming it

},
}

impl TryFrom<(u32, Vec<u8>)> for ExtensionCoreImpl {
Copy link
Member

Choose a reason for hiding this comment

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

actually should just use the Decode macro from parity_scale_codec to auto generate the decode logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so query_method is also redundant. The call details will embed in guest args.

@indirection42
Copy link
Contributor Author

The benefits of having indirect Enum implementations for extension functions are that we can implement some functionals later, like querying fees of the functions, right?

@indirection42 indirection42 requested a review from xlc June 5, 2024 10:31
@indirection42 indirection42 marked this pull request as ready for review June 5, 2024 10:47
@indirection42
Copy link
Contributor Author

I found accessing context in func_wrap is not trivial. One way is accessing it via self which is context, but due to the constraint of the linker.func_wrap, Fn closure should be sync, so captured variable invoke_source should be wrapped in Arc. Another way is accessing it via caller.data, it's also context, but it's a generic, we need to add trait bounds with getter to access the invoke_source.

@xlc
Copy link
Member

xlc commented Jun 5, 2024

add trait bounds is fine

poc/extensions/src/extension_core.rs Outdated Show resolved Hide resolved
poc/extensions/src/perm_controller.rs Outdated Show resolved Hide resolved
@indirection42 indirection42 requested a review from xlc June 6, 2024 02:38
poc/extensions/src/extension_fungibles.rs Outdated Show resolved Hide resolved
}

impl<Impl: ExtensionFungibles> ExtensionId for ExtensionFungiblesCall<Impl> {
const EXTENSION_ID: ExtensionIdTy = Impl::EXTENSION_ID;
Copy link
Member

Choose a reason for hiding this comment

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

extension id should be generated by this macro

use core::marker::PhantomData;
use parity_scale_codec::{Decode, Encode};

pub trait ExtensionFungibles: ExtensionId {
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 trait ExtensionFungibles: ExtensionId {
pub trait ExtensionFungibles {

it is not up to the implementor of ExtensionFungibles to determine the extension id


struct Context<E: ExtensionTuple, P: PermController> {
invoke_source: InvokeSource,
phantom_p: PhantomData<P>,
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
phantom_p: PhantomData<P>,
_marker: PhantomData<(P, E)>,

so we don't need two of it

@indirection42 indirection42 requested a review from xlc June 6, 2024 03:13
@indirection42 indirection42 merged commit f1b7052 into master Jun 6, 2024
2 checks passed
@indirection42 indirection42 deleted the poc-extension-system branch June 6, 2024 03:53
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.

PoC: Extension System
2 participants