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

Draft
wants to merge 65 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

/// The type implementing the runtime query dispatch.
pub ty: T::Type,
/// The query interfaces metadata.
pub interfaces: Vec<QueryInterfaceIR<T>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't pallet view functions scoped to certain pallets? So would it make sense for this information to be inside the PalletMetadataIR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR allows them to be defined only as part of pallets (because it is convenient for the initial implementation), but the requirement is to make them pallet agnostic, so they can be defined independently of pallets.

Copy link
Contributor

@jsdw jsdw Nov 8, 2024

Choose a reason for hiding this comment

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

What's the advantage/reason to replace Runtime APIs in the future with these? My understanding (which might miss something; I haven't read the background docs in a while not) is:

  • We have "Runtime APIs" as a pallet agnostic set of APIs (called externally via eg state_call by using RuntimeApiTraitName ++ _ ++ method_name and SCALE encoding the args.
  • We have here "Pallet View Functions", which are like Runtime APIs but are defined at the pallet level instead (and queried via a Runtime API call). These currently are accessed by using twox_128(pallet_name) ++ twox_128("fn_name(fnarg_types) -> return_ty") and SCALE encoding the args.

Both allow you to call functions and get the results back. In the future I'd expect both to become subscribable. The calling convetion of Pallet View Functions (hashing the type sig etc to query them) is different in order to enforce the type signature in the query (which means that changing the type signature of one would lead to old queries being invalid rather than being executed and complaining about wrongly encoded params or whatever).

I guess overall I see them as working alongside Runtime APIs but at the pallet level, which is also why I've tended to push for making them consistent with Runtime APIs in terms of how they are talked about and called etc so far, but I feel like I am missing something because others seem less keen on this so far :D

\cc @kianenigma maybe you also have some insight/thought I'm missing here :)

But all in all, I am happy to go ahead with whatever naming etc and figure it out in followup PRs anyways!


/// Metadata of a runtime query interface.
#[derive(Clone, PartialEq, Eq, Encode, Debug)]
pub struct QueryInterfaceIR<T: Form = MetaForm> {
Copy link
Contributor

@jsdw jsdw Nov 7, 2024

Choose a reason for hiding this comment

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

How much could we align this with Runtime APIs in terms of the naming of structts/fields?

For runtime APIs we have:

RuntimeApiMetadataIR { name, methods, docs, deprecation_info }
RuntimeApiMethodMetadataIR { name, inputs, output, docs, deprecation_info }
RuntimeApiMethodParamMetadataIR { name, ty }

So here, we could, per pallet, have something like:

struct PalletApiMethodMetadataIR { name, id?, inputs: Vec<PalletApiMethodParamMetadataIR>, output, docs, deprecation_info }
struct PalletApiMethodParamMetadataIR { name, ty }

(I went for Api instead of ViewFunction just because I prefer it to viewFunctions and it's shorter, but it's all good!)

I guess having the id field lets us avoid having to construct it, since it's a bit arduous hashing the relevant bits, so makes sense for now! Personally I'd simplify the way we call these things to be more like runtime APIs, but I'm happy to propose that as a separate PR after this merges!

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 agree should try to align the metadata with Runtime APIs...in theory we should be able to replace Runtime APIs in the future with "view functions".

As for naming...it would be good to come up with something consistent: as you can see I am using "query" interchangeably with "view_function". API is okay but maybe too generic and could be confused with existing Runtime API? Naming is hard, any suggestions welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally prefer Api to align with Runtime APIs, but happy for everything to be named consistently with any of Api, Query, ViewFunc (just to help shorten a touch) or ViewFunction. For the sake of being able to rename it all quite easily in a followup PR if we like, perhaps one of the latter two is ideal (easy to search and replace ViewFunc/ViewFunction!)

}

impl<#type_impl_gen> #query_struct_ident<#type_use_gen> #where_clause {
/// Create a new [`#query_struct_ident`] instance.
Copy link
Contributor

@jsdw jsdw Nov 8, 2024

Choose a reason for hiding this comment

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

Nit: this outputs as written here rather than #query_struct_ident being substituted with the ident as you'd want.

Best bet might be to build the doc string beforehand and then use #[doc = #doc_string] to get it in!

@jsdw jsdw mentioned this pull request Nov 11, 2024
@jsdw
Copy link
Contributor

jsdw commented Nov 13, 2024

This is generally looking good to merge to me now! It's behind an view_functions_experimental which gives us some room to tweak things in followup PRs already. A couple of small outstanding bits from me:

  • Document the TODOs.
  • prdoc
  • Consistify query/view_function related language to all be view_function/ViewFunction. A bit verbose but makes it easy to search and replace if we want to change the name before stabilising things.
  • MetadataIR: I'd move the view function stuff into pallets rather than haev a new top level section for it. If needed this can be a followup PR though!

Thw top 3 of those would be enough for a tick from me :) It'd be good to also have a FRAME/Runtime dev have a look over since I'm newer to this code.

@ascjones
Copy link
Contributor Author

Okay thanks @jsdw, I will attempt to resolve these by the end of next week.

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