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

Implement pallet view function queries #4722

Open
wants to merge 139 commits into
base: master
Choose a base branch
from

Conversation

ascjones
Copy link
Contributor

@ascjones ascjones commented Jun 6, 2024

Closes #216.

This PR allows pallets to define a view_functions impl like so:

#[pallet::view_functions]
impl<T: Config> Pallet<T>
where
	T::AccountId: From<SomeType1> + SomeAssociation1,
{
	/// Query value no args.
	pub fn get_value() -> Option<u32> {
		SomeValue::<T>::get()
	}

	/// Query value with args.
	pub fn get_value_with_arg(key: u32) -> Option<u32> {
		SomeMap::<T>::get(key)
	}
}

QueryId

Each view function is uniquely identified by a QueryId, which for this implementation is generated by:

twox_128(pallet_name) ++ twox_128("fn_name(fnarg_types) -> return_ty")

The prefix twox_128(pallet_name) is the same as the storage prefix for pallets and take into account multiple instances of the same pallet.

The suffix is generated from the fn type signature so is guaranteed to be unique for that pallet impl. For one of the view fns in the example above it would be twox_128("get_value_with_arg(u32) -> Option<u32>"). It is a known limitation that only the type names themselves are taken into account: in the case of type aliases the signature may have the same underlying types but a different id; for generics the concrete types may be different but the signatures will remain the same.

The existing Runtime Call dispatchables are addressed by their concatenated indices pallet_index ++ call_index, and the dispatching is handled by the SCALE decoding of the RuntimeCallEnum::PalletVariant(PalletCallEnum::dispatchable_variant(payload)). For view_functions the runtime/pallet generated enum structure is replaced by implementing the DispatchQuery trait on the outer (runtime) scope, dispatching to a pallet based on the id prefix, and the inner (pallet) scope dispatching to the specific function based on the id suffix.

Future implementations could also modify/extend this scheme and routing to pallet agnostic queries.

Executing externally

These view functions can be executed externally via the system runtime api:

pub trait ViewFunctionsApi<QueryId, Query, QueryResult, Error> where
	QueryId: codec::Codec,
	Query: codec::Codec,
	QueryResult: codec::Codec,
	Error: codec::Codec,
{
	/// Execute a view function query.
	fn execute_query(query_id: QueryId, query: Query) -> Result<QueryResult, Error>;
}

XCQ

Currently there is work going on by @xlc to implement XCQ which may eventually supersede this work.

It may be that we still need the fixed function local query dispatching in addition to XCQ, in the same way that we have chain specific runtime dispatchables and XCM.

I have kept this in mind and the high level query API is agnostic to the underlying query dispatch and execution. I am just providing the implementation for the view_function definition.

Metadata

Currently I am utilizing the custom section of the frame metadata, to avoid modifying the official metadata format until this is standardized.

vs runtime_api

There are similarities with runtime_apis, some differences being:

  • queries can be defined directly on pallets, so no need for boilerplate declarations and implementations
  • no versioning, the QueryId will change if the signature changes.
  • possibility for queries to be executed from smart contracts (see below)

Calling from contracts

Future work would be to add weight annotations to the view function queries, and a host function to pallet_contracts to allow executing these queries from contracts.

TODO

  • Consistent naming (view functions pallet impl, queries, high level api?)
  • End to end tests via runtime_api
  • UI tests
  • Mertadata tests
  • Docs

@re-gius re-gius requested a review from a team as a code owner January 14, 2025 17:03
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

Why didn't we implement this similar to RuntimeCall or RuntimeTask?
We could make like RuntimeCall, have it as an enum, IMO it looks simpler but we can keep as it is.

EDIT: reading from discussion above it can be because of the 256 variant limit.

We don't need to put it as an associated type in frame system AFAICT no? because we will never use it in abstract logic.

EDIT: Because the code is good, I suggest we just remove the RuntimeViewFunction type, and associated type.
in the construct_runtime expansion we implement the execute_view_function directly on the the Runtime type no need to have the inner call to RuntimeViewFunction.
EDIT: I am also open to others opinion.

NOTE: we could also make all pallet implement the trait ExecuteViewFunction and use the type AllPalletsWithSystem and implement it via executive.

Cargo.lock Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
cumulus/pallets/weight-reclaim/src/tests.rs Show resolved Hide resolved
let view_functions_custom_metadata = CustomValueMetadata {
ty: ir.view_functions.ty,
value: codec::Encode::encode(&view_function_groups),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this code, CustomValueMetadata is supposed to give a type and a value of the given type no? here we give as type RuntimeViewFunction which is an enum with zero variant AFAICT, and as value we give a vector of ViewFunctionGroupIR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the reasoning here has been something like: show users similar metadata to RuntimeCall and hide the hashing-based dispatching mechanism and all the complexity from them, so that they are shown something they are familiar with.

Copy link
Contributor

Choose a reason for hiding this comment

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

So hiding the dispatching-based mechanism and offering a stable and readable interface

Copy link
Contributor

@gui1117 gui1117 Jan 23, 2025

Choose a reason for hiding this comment

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

I am not sure, here is the doc:

pub struct CustomValueMetadata<T: Form = MetaForm> {
	/// The custom type.
	pub ty: T::Type,
	/// The custom value of this type.
	pub value: Vec<u8>,
}

If we provide the type RuntimeViewFunction which is a variant-less enum, a UI cannot decode the value without hard-coding the format of the value.

But anyway we will see if I am right or wrong when we start using it and we can change later.

EDIT: if later we change the type from hash stuff to an enum, I would expect to change the type here too.

substrate/frame/system/src/lib.rs Outdated Show resolved Hide resolved
let view_function_groups = registry.map_into_portable(ir.view_functions.groups.into_iter());
let view_functions_custom_metadata = CustomValueMetadata {
ty: ir.view_functions.ty,
value: codec::Encode::encode(&view_function_groups),
Copy link
Contributor

Choose a reason for hiding this comment

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

As a note: view_function_groups is of type ViewFunctionGroupIR so when the IR the type here will change too. maybe it can be better to have a more fixed type, but it can be fine.

fn try_from(method: syn::ImplItemFn) -> Result<Self, Self::Error> {
let syn::ReturnType::Type(_, type_) = method.sig.output else {
return Err(syn::Error::new(
method.sig.output.span(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious if when the output is nothing, if the span is correct or just fall back to call site.

Copy link
Contributor

@re-gius re-gius Jan 20, 2025

Choose a reason for hiding this comment

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

I'm not sure I understand this comment, can you provide more context please?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we write fn foo() {} there is no output, so I am afraid that method.sig.output.span() will return call site span, making the error message less helpful.

If you want to test, you can implement some ui tests in substrate/frame/support/tests/test/pallet_ui/

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

Apart from the current comments, looks good to me

re-gius and others added 21 commits January 16, 2025 11:52
…ions.rs

Co-authored-by: Guillaume Thiolliere <guillaume.thiolliere@parity.io>
Co-authored-by: Guillaume Thiolliere <guillaume.thiolliere@parity.io>
Co-authored-by: Guillaume Thiolliere <guillaume.thiolliere@parity.io>
Co-authored-by: Guillaume Thiolliere <guillaume.thiolliere@parity.io>
Co-authored-by: Guillaume Thiolliere <guillaume.thiolliere@parity.io>
@ascjones
Copy link
Contributor Author

Thank you @re-gius for all the hard work finishing up this PR 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework. T4-runtime_API This PR/Issue is related to runtime APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FRAME Core] Add support for view_functions