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 47 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

@athei
Copy link
Member

athei commented Sep 18, 2024

This just reads as if you agree with @joepetrowski and @bkchr that runtime level view functions should behave like runtime APIs: You define a signature/trait and then each runtime decides how to implement it.

@bkchr
Copy link
Member

bkchr commented Sep 18, 2024

EVM chain will use ERC20 to implement it and AH will use pallet-assets.

And if a chain uses both of these pallets? :D

Generally I think that the pallet local stuff is quite easy and can be automatically exposed when adding a pallet to a runtime.

For runtime level/when you need multiple pallets having some automatic implementation is not that easy. We could maybe have some fake pallets/runtime api implementation providers which provide the implementation based on multiple pre-defined pallets.

@athei
Copy link
Member

athei commented Sep 18, 2024

Agreed. Yes the runtime level part is much harder. So maybe we can break it down in two PRs and focus only on the pallet wide view functions in this PR since the design there is mostly done.

But I think we should decide on how to identify view functions now (hash vs index). I agree with @kianenigma that we should dispatch based on enum index . Not only to keep it uniform with Call but also because I realized that hashes don't really offer too much benefits: Yes they are robust against moving pallets around in the runtime but we can't do that anyways because of Call. Besides, we have index attributes to address this problem. They also don't really remove the need to look up information in the Metadata as you need to learn about the arguments anyways. So you might as well look up the index of the view function while you are there. Lastly, hashes prevent renaming of the view function which can be useful in rare occasions.

tl;dr We should use the same index/enum based dispatch mechanism as Call. But the index attribute should be mandatory on the view function. Would be weird to index based on ordering inside the pallet. @bkchr What do you think?

We should also reserve the first entry in the outer enum for runtime level view functions (which will be implemented later):

enum ViewFunction {
    RuntimeWide(u32) = 0,
    System(u32),
    Balances(u32),
    ....
}

@ascjones
Copy link
Contributor Author

Yes they are robust against moving pallets around in the runtime but we can't do that anyways because of Call. Besides, we have index attributes to address this problem.

Different runtimes have different pallet configurations, so if a multichain UI wanted to use a standard view function it is not guaranteed to have the same enum index. However, client libs could do a string lookup for e.g. "Balances" in the metadata to determine the index.

Another benefit would be the inbuilt "versioning", a new signature would just have a different id, although this is not foolproof without proper type information.

That being said, I wouldn't be against enum based indices since it is simpler to implement, but pushing a bit of complexity to client libs.

@athei
Copy link
Member

athei commented Sep 18, 2024

Different runtimes have different pallet configurations, so if a multichain UI wanted to use a standard view function it is not guaranteed to have the same enum index. However, client libs could do a string lookup for e.g. "Balances" in the metadata to determine the index.

It is a good point. Client libraries will probably use metadata to generate the view function accessors. When using indices they will have to generate one set of functions per chain. When using hashes they can just generate functions from all the metadata files they have access to and merge them while removing duplicates. I think thats much better. Assuming that pallet names are unique of course.

And even when using generated code people will refer to the function by name and signature. Changing the name will break peoples code when they upgrade the lib as it will have generated the function under a different name. So we might as well encode it into the id to make sure we never ever change it.

So yes I think hashing based on name + signatures makes it easier for client lib devs. I think it outweighs the benefits of having everything done the same way. What do you think @kianenigma ?

@bkchr
Copy link
Member

bkchr commented Sep 18, 2024

When using hashes they can just generate functions from all the metadata files they have access to and merge them while removing duplicates. I think thats much better. Assuming that pallet names are unique of course.

I don't really get what you are saying here? Why do they need multiple functions? For what?

Pallet names are not unique across different chains, you can name your pallet whatever you like in construct_runtime.

@kianenigma
Copy link
Contributor

You are mixing here runtime level with pallet level. If every pallet is exposing its own view function, how could they be missed?

@bkchr: They can be, because you might forget to put the right piece of code in impl_runtime_api. As in, even if we only have pallet-level view fns, you have to add this to your runtime:

https://github.com/ascjones/polkadot-sdk/blob/21eeba75e94cf6408c7ae1843f94142ce8f338af/substrate/bin/node/runtime/src/lib.rs#L2687-L2691

Agreed. Yes the runtime level part is much harder. So maybe we can break it down in two PRs and focus only on the pallet wide view functions in this PR since the design there is mostly done.

@athei: Second. It is hard to foresee things without getting the hands dirty.

This PR is stable enough to be built on top of. Any of us can and should go and build a few pallet-level view fns on top of this PR, and then see how it will look like if we want to amalgamate them. The view function forum post has some ideas in it both for pallet level, and runtime level view functions.

So yes I think hashing based on name + signatures makes it easier for client lib devs. I think it outweighs the benefits of having everything done the same way. What do you think @kianenigma ?

Sadly both pallet name and index are variable per-runtime, so the north star of removing complexity from client libs is not feasible. In all future scenarios, if a client lib wants to provide an abstraction over the same view function in 5 different chains, it would inevitably need to go through some effort and "speculate/hardcode" a few things. I don't think there is any solution for it.

Given this, I am also still leaning towards enum based dispatch. It is more unified, and we can pursue someday moving all dispatches to hashes, but not have one foot in each. With the time bomb of reaching the 255 limit, we will have to do it someday :)

We can then reserve a special index in both the RuntimeQueryId, and RuntimeCall and so on, to extend all Runtime* enums at the runtime level. This solves both defining a runtime-level Query, and a Call and so on.

@kianenigma
Copy link
Contributor

kianenigma commented Sep 19, 2024

Further comment on stability:

💡 in fact the only thing that is not variable across runtimes is the "original" (prior to cargo package renaming) crate-name of a pallet. Do we put this in the metadata? perhaps we should, and then client libs can have an easier time building abstraction. They can abstract away over pallet index or pallet name, and instead say look for an instance of "a pallet built from pallet-balances crate, no matter the name or index".

Of course, you would still have the complexity of pallet instancin, but I think working on the basis of crate name is the only reasonable step forward here.

All in all, the ordering seems to be, from "unstable" to "stable":

Pallet index << Pallet Name << Pallet Crate name

@jsdw
Copy link
Contributor

jsdw commented Sep 19, 2024

in fact the only thing that is not variable across runtimes is the "original" (prior to cargo package renaming) crate-name of a pallet.

I think that identifying a pallet by the pallets name is still the way to go for client libs. This allows a crate to be reused with different config for more than one pallet without clients caring (which was done not too long ago for some pallet I can't remember off the top of my head).

I'm also happy to stick with enum index over hash; we've had to solve this problem already for eg RuntimeCalls on the client side (which was can do by looking up the correct index based on the name of the pallet / call that we want) so offhand I don't mind the same being true for making these view function calls, and don't feel like it will add a lot of complexity on the client side.

The 256 limit scares me a bit, but I expect we can migrate things over to hash based lookup by the time we get close to that (which may come more quickly once each top level call gets its own variant).

@shawntabrizi
Copy link
Member

shawntabrizi commented Sep 19, 2024

@bkchr has said the solution to the 256 limit problem actually is to use compact integers for the enum length, whereas we use a single byte right now. But this is a big breaking change to SCALE and our ecosystem.

That would support enums of arbitrary length.

That being said, using a small hash probably brings to our ecosystem a lot of familiarity and devex to Polkadot, as Ethereum uses this pattern in smart contracts, and it allows ways to uniquely identify certain kinds of calls and functions across pallets and implementations.

That being said, in the Rust ecosystem, we do have traits, and it seems that this would solve the same problem if we actually used them right.

@athei
Copy link
Member

athei commented Sep 20, 2024

I was assuming we are using the create name as pallet name when writing my text. Given that it should be stable across chains. Keep in mind we are also hashing the signature in. This means the hash is the same across chains as changing the view functions semantic is not allowed.

I see really no benefit in using indices except not introducing a new way of dispatching.

@jsdw
Copy link
Contributor

jsdw commented Oct 3, 2024

Just asking couple of possibly DQs as I try to understand this area/direction better :):

  1. What's the difference between "runtime view functions" and our existing "runtime APIs"?
    • Do we need both or we tweak/improve Runtime APIs to get to where we want? (Personally I'd rather have one approach over two if possible)
    • I guess one "difference" is the way that they might be made immutable ie hashing function name and args to prevent either from changing, whereas runtime APIs have a different versioning approach?
    • I guess thing we want to improve/add is tracking storage accesses so that we can have subscribable runtime APIs?
    • There was other talk about generating the code for them, but then the counterpoint that we want to provide a single stable interface at the runtime level which can use potentially different pallets etc under the hood to hand back the desired values. I'm thinking then that codegen is probably out of scope at the moment (and perhaps something we can think about adding later to eg make exposing runtime fns using common pallets easy)?
  2. What still needs doing to push this PR over the finish line?
    • I think there was agreement on hash based querying as implemented (this makes sense to me given the conversation so far)?
    • Do we need to add something around tracking storage accesses or can that be added as a future PR so that we can aim to merge this initial work sooner?
    • I think it's OK that this puts things in the custom metadata field for now; we can add better support in V16 metadata as a separate PR when this merges.
  3. How much do we care about all pallet level view functions (or runtime view functions) being "immutable"?
    • We need some stability to form a solid foundation to build higher level facade APIs on, but do we also want to be able to expose "unstable" functions (perhaps indicated in the metadata) to allow us to experiment and iterate on some functions until we decide that they are stable? (The fact that we expose an unstable metadata is super useful for instance, and lets us iterate quickly and play around with what needs to go in until we stabilise).
    • Do we want to have this stability guarantee at pallet and runtime level? Pallet level view functions rely on pallet names (and runtimes can rename pallets and have multiple of the same pallet in) which feels like a shakier place to try and have stability guarantees at than runtime level.

@athei
Copy link
Member

athei commented Oct 4, 2024

What's the difference between "runtime view functions" and our existing "runtime APIs"?

It is some infrastructure built on top of runtime APIs to allow for easy definition and consumption of queries. Assuming just the pallet level functions as defined in this PR: It allows for the definition inside the pallet without the cumbersome declaration and later implementation at runtime level. It is just shipped with the pallet. The other feature is immutability: They are never ever allowed to be removed or altered. This is not the case for runtime APIs in general.

What still needs doing to push this PR over the finish line?

Settle the debate on hashes vs index. I don't think we need to do anything for subscribe ability in this PR. We can add an RPC later like state_call_subscribe which can just subscribe to any runtime API.

How much do we care about all pallet level view functions (or runtime view functions) being "immutable"?

A lot. It is almost the whole point of it. So the will be stable. Period. We don't break downstream tools on purpose. So this is also why we suggest hashing the crate name and not the name within the runtime.

@jsdw
Copy link
Contributor

jsdw commented Oct 4, 2024

Thanks for your reply @athei!

Mayeb I'm way off base or derailing things and apologies if so, but just some thoughts and responses:

What's the difference between "runtime view functions" and our existing "runtime APIs"?

It is some infrastructure built on top of runtime APIs to allow for easy definition and consumption of queries.

Does this mean that somebody will be able to define runtime APIs and runtime view functions and then access either via eg state_call?

What still needs doing to push this PR over the finish line?

Settle the debate on hashes vs index. I don't think we need to do anything for subscribe ability in this PR. We can add an RPC later like state_call_subscribe which can just subscribe to any runtime API.

Ah ok, good to know we can leave subscribing for another PR. I geuss the same applies when we impl runtime level view functions; subscribe-ability can be a later followup hopefully!

How much do we care about all pallet level view functions (or runtime view functions) being "immutable"?

A lot. It is almost the whole point of it. So the will be stable. Period. We don't break downstream tools on purpose. So this is also why we suggest hashing the crate name and not the name within the runtime.

Yup 100%; having stable functions whose signature/meanings do not change makes sense for sure. I guess I am just questioning:

  1. Where do we define this stable interface?
    • Should we have a specified and stable interface at the pallet level and runtime level, or would it make more sense to define/specify a stable interface only at the runtime level? Then we don't need to care about pallet names, or even which pallets exist at all. We have a runtime-level API spec which can be implemented behind the scenes from any pallets that make sense.
    • If we use crate name to reference the pallet at pallet level, what do we do if the same pallet/crate is used more than once in a runtime, and what do I do if I want to write a new pallet which is compatible with this pallet level spec but is a different crate.
  2. Does the stability of the interface need to be defined/enforced in the code?
    • Accessing a function by constructing and hashing a string like pallet_name + name(arg1,arg2) -> returntype is more cumbersome than pallet_name + name. What we gain is that in order to implement a function from the stable spec, it must have the correct signature (barring type aliases or whatever). We still need to specify (in writing somewhere) the expected semantics though. So we could just specify the name/hash and the semantics somewhere and keep the accessing logic simpler?
    • Does a more complex hashing scheme help protect against breaking the spec in a runtime update? In the more complex scheme the function becomes inaccessible if the type changes. In a simpler scheme, it no longer follows the semantics properly if broken. Either way, we could perhaps think about having a test suite or something to help runtimes check that they are adhering to the spec, but otherwise it'll rely on humans not breaking the stable interface when they update things. So I'm not sure what advantage the more complex hashing brings.

I suppose in the end, I feel like in order to have a stable interface, we'd need to define a written spec anyway for runtimes to adhere to. The spec could break the interface down into sections for eg staking, governance, assets or whatever, a bit like https://paritytech.github.io/json-rpc-interface-spec/ does, to allow runtimes to adhere to one or more sections but not necessarily everything.

if we have such a spec, we could expose runtime-API-like functions that adhere to it, and have a test suite to help test for conformance on a runtime. Pallet level view functions would be called in this interface to assemble the information we want to hand back (and avoid direct storage requests as much as possible), but they themselves would be an implementation detail w.r.t the spec, so it would be totally pallet agnostic. We also wouldn't really need any special hashing of name(args)->returntype etc (though we could have that easily enough if we wanted, though I wouldn't see the point for pallet level fns); we'd define whatever names/hashes/semantics in the spec and that is the source of truth for any runtime that wants to adhere to it. We could copy the JSON-RPC V2 spec in a few ways if we wanted too, eg verioned names like Balances_unstable_get_balance for unstable fns and then stabilise to become Balances_v1_get_balance when we are happy with that part of the interface.

Maybe I'm going way off base here though and missing some crucial bits; what do you reckon?

@athei
Copy link
Member

athei commented Oct 4, 2024

Does this mean that somebody will be able to define runtime APIs and runtime view functions and then access either via eg state_call?

A view function is a runtime API. So yes, both are accessed via state_call.

Regarding the stability stuff: Let's not over engineer here. There is a lot of code if you would change it, everything would fall apart. The code is the definition and we just not change the semantics. Just like for dispatchables. I mean what for do we have a runtime if we still need a spec? CI checks that detect breakage is welcome, though.

@jsdw
Copy link
Contributor

jsdw commented Oct 8, 2024

The code is the definition and we just not change the semantics. Just like for dispatchables. I mean what for do we have a runtime if we still need a spec?

My thinking is that having a spec comes in handy when we want multiple runtimes to implement the same sets of methods and semantics in order that the facade stuff can talk to them all. It's a single source of truth from which we can then also have eg CI tests or whatever to check for conformance. So then any runtime can choose to impl the spec or not (or sections of it) and we can have tests/tools to help check this, and help ensure that support isn't later broken by a runtime code change.

If the higher level facade APIs are just thin wrappers around this spec (ignoring historical stuff), then we might also be able to define both the expected runtime APIs and then higher level facade APIs and semantics in one go.

@athei
Copy link
Member

athei commented Oct 8, 2024

Multiple runtimes importing the same pallets will offer the same semantics. Thats the whole point of bundling them into modules (aka pallets) and not implementing them at runtime level like a generic runtime API.

And yes runtime wide view functions will put a slight wrinkle into this plan. But we can also just package them into a crate that we distribute and that everybody imports and wires them up with their pallets.

Maybe you can view that as some sort of spec.

@kianenigma
Copy link
Contributor

Regarding the hashing: no one is strongly backing it, we should set our aim to merge this PR with old school dispatch. This method also has not benefit per-se, but it is at least more consistent.

Some final counterarguments:

  1. There is some engineering effort needed to migrate this PR to enum dispatch. I don't think it is massive.
  2. Hashes will not have this 256 limit. At the RuntimeQuery (RuntimeView), we will only hit the limit when a runtime has more than 255 pallets, so same time as all Runtime* explode. At the pallet level, one might argue that a pallet might normally have waaaaaaaaaaay more view functions than it would have storage items, but I don't see that. A good pallet probably has max a dozen storage, and a dozen view functions.

Having all this conversation is a good encouragement to already start thinking about a migration plan into compact encoded indices for enums to eliminate this time bomb.

@jsdw
Copy link
Contributor

jsdw commented Oct 9, 2024

And yes runtime wide view functions will put a slight wrinkle into this plan. But we can also just package them into a crate that we distribute and that everybody imports and wires them up with their pallets.

Maybe you can view that as some sort of spec.

This all makes sense, and yup this is the bit that I'm thinking about as a spec, and I agree that the spec can be implemented in the form of code and probably from this we can generate documentation to also describe it to people who want to then use the facade interface at a higher level (client facade implementations for instance)

@jsdw
Copy link
Contributor

jsdw commented Oct 9, 2024

Just a general thought on the naming in this PR:

How do people feel about calling these "Pallet APIs" rather than "Pallet view functions"? My thought is to be consistent with the term "Runtime APIs", which (from the client perspective at least) act very similar; read-only functions that can be called with some arguments and return some value back.

@jsdw
Copy link
Contributor

jsdw commented Oct 10, 2024

Regarding the hashing: no one is strongly backing it, we should set our aim to merge this PR with old school dispatch. This method also has not benefit per-se, but it is at least more consistent.

@kianenigma I was thinking what the metadata would look like in order that clients know how to call pallet view functions.

For the case of eg transactions, we provide an enum in the metadata for each pallet, and that works great because each variant name corresponds to the transaction name and each enum field correpsonds to one of the transaction arguments.

For the case of Pallet View Functions (same as rutime APIs), an enum doesn't provide enough information (ie we also need to know the return type for each Pallet View Function. So I suppose the metadata for the ciew functions in a pallet using enum dispatch would look more like:

view_functions: vec![
    PalletViewFunction {
        index: 1u8,
        name: String,
        inputs: vec![12, 34, 8], // u32 type IDs for each argument
        output: 34, // u32 type ID for return type,
        docs: Vec<String>
    },
    // ...
]

The index in the metadata exists only to dispatch to the right function using the enum approach. If we did hash based dispatch, then we could omit the index and describe pallet view functions identically to Runtime APIs, ie like this:

pub struct PalletApiMethodMetadata<T: Form = MetaForm> {
	/// Method name.
	pub name: T::String,
	/// Method parameters.
	pub inputs: Vec<RuntimeApiMethodParamMetadata<T>>,
	/// Method output.
	pub output: T::Type,
	/// Method documentation.
	pub docs: Vec<T::String>,
}

(see https://github.com/paritytech/frame-metadata/blob/main/frame-metadata/src/v15.rs#L132)

Does this change your opinion on enum versus hash based dispatch, or is it actually a non issue?

@athei
Copy link
Member

athei commented Oct 11, 2024

How do people feel about calling these "Pallet APIs" rather than "Pallet view functions"? My thought is to be consistent with the term "Runtime APIs",

I don't think we should use bad names for the sake of consistency. Remember that we call host functions "Runtime Interface". It is confusing to use those generic names. There will also be runtime wide view functions eventually.

@ascjones
Copy link
Contributor Author

ascjones commented Oct 15, 2024

Regarding the hashing: no one is strongly backing it, we should set our aim to merge this PR with old school dispatch. This method also has not benefit per-se, but it is at least more consistent.

Throwing some thoughts out there:

  • Inspiration for the hashing is from solidity/ink! selectors, allows defining common standards: a known set of selectors. e.g. NFT wallets/UIs can identify the ERC721 selectors.
  • Isn't enum based dispatch just a legacy implementation detail? The Outer runtime enum and inner pallet enum are both macro generated, and the derived SCALE decoding is used for the correct dispatching. Hence we inherit the limited u8 + u8 address space. The important part is the Runtime wide guarantee of uniqueness of each dispatchable's prefix

Sadly both pallet name and index are variable per-runtime, so the north star of removing complexity from client libs is not feasible. In all future scenarios, if a client lib wants to provide an abstraction over the same view function in 5 different chains, it would inevitably need to go through some effort and "speculate/hardcode" a few things. I don't think there is any solution for it.

  • No data to back this up but more often than not the default pallet name is used, so by default most view functions will have the standard hash. In cases where it is not we could allow specifying a custom prefix in construct_runtime to ensure consistency.
  • Another approach would be to instead of twox_128(pallet_name) ++ twox_128("fn_name(fnarg_types) -> return_ty"), just use the hashed signature with an optional prefix prefix ++ twox_128("fn_name(fnarg_types) -> return_ty"). Prefix provides uniqueness in case of identical fn signatures and could be defined in view fn definition to be pallet agnostic. Downside would be O(n) lookups for view functions, but with a low n that won't be too bad?
  • Using enum based lookup makes the smart contract use case for querying view functions difficult, since the enum prefixes/definitions will need to be hardcoded. Looking them up would be too much overhead.

Given this, I am also still leaning towards enum based dispatch. It is more unified, and we can pursue someday moving all dispatches to hashes, but not have one foot in each. With the time bomb of reaching the 255 limit, we will have to do it someday :)

I like the idea of things being unified, OTOH if we know we are going there, why not do it here first where the impact is low (nobody is using view functions yet). The cost of migrating in the future should be considered.

@kianenigma
Copy link
Contributor

I like the idea of things being unified, OTOH if we know we are going there, why not do it here first where the impact is low (nobody is using view functions yet). The cost of migrating in the future should be considered.

This is a great idea, and a nice bonus: If you think we can already experiment with Enum based dispatch, with compact encoded indices, let's do it here. @ascjones

For the case of Pallet View Functions (same as rutime APIs), an enum doesn't provide enough information (ie we also need to know the return type for each Pallet View Function.

Indeed, the metadata for view functions would need to be more sophisticated than dispatch, so it cannot be purely enum. @jsdw

How do people feel about calling these "Pallet APIs" rather than "Pallet view functions"? My thought is to be consistent with the term "Runtime APIs",
I don't think we should use bad names for the sake of consistency. Remember that we call host functions "Runtime Interface". It is confusing to use those generic names. There will also be runtime wide view functions eventually.

About naming, I would go with view functions for now. We can always rename it later and pre-launch as well, so it is okay.

@ascjones
Copy link
Contributor Author

I like the idea of things being unified, OTOH if we know we are going there, why not do it here first where the impact is low (nobody is using view functions yet). The cost of migrating in the future should be considered.

This is a great idea, and a nice bonus: If you think we can already experiment with Enum based dispatch, with compact encoded indices, let's do it here. @ascjones

I was in fact referring to "we can pursue someday moving all dispatches to hashes", so presenting the case for using hash instead of enum based dispatch 🙈

@jsdw
Copy link
Contributor

jsdw commented Oct 25, 2024

Me and @kianenigma had a chat about this the other day and the consensus was that, while our opinions differ on enum dispatch versus not, we shouldn't let merging this PR get too held up on this point.

If there are concerns about merging this and then later altering it, then perhaps we could mark it as unstable like we do with new metadata versions and call the attr #[pallet::unstable_view_functions] with appropriate documentation, to give us the ability to merge and experiment with this and then iterate with smaller PRs if we want to update anything to reach consensus.

Does that work for everybody here? Should we mark as "unstable" eg like above or can we review and merge and still tweak it later without this?

@athei
Copy link
Member

athei commented Oct 25, 2024

yes

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
10 participants