-
Notifications
You must be signed in to change notification settings - Fork 242
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
Parameterize AccountData
#409
Conversation
examples/custom_config.rs
Outdated
impl Config for MyConfig { | ||
type Index = <DefaultConfig as Config>::Index; | ||
type BlockNumber = <DefaultConfig as Config>::BlockNumber; | ||
type Hash = <DefaultConfig as Config>::Hash; | ||
type Hashing = <DefaultConfig as Config>::Hashing; | ||
type AccountId = <DefaultConfig as Config>::AccountId; | ||
type Address = <DefaultConfig as Config>::Address; | ||
type Header = <DefaultConfig as Config>::Header; | ||
type Signature = <DefaultConfig as Config>::Signature; | ||
type Extrinsic = <DefaultConfig as Config>::Extrinsic; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for the sake of the example, could we actually override one or more of these types perhaps (maybe then with a comment saying what's going on), just to make it clear that that's sortof the point of this impl? (I realise that doing so may prevent it from actually working below; something that would want to be made clear also.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wee suggestion re the example but looks great!
I tried using your example and ran into this:
|
examples/custom_config.rs
Outdated
#[derive(Clone, Debug, Default, Eq, PartialEq)] | ||
pub struct MyConfig; | ||
impl Config for MyConfig { | ||
type Index = u64; // this is different from the default `u32` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this; worth noting that the example won't run unless this is set back to the default though I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example does run though, I guess because u64 : From<u32>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah; I assumed that @trusch's comment meant otherwise!
That is deliberate (but should be emphasised); I think it's worth showing how you'd actually customise the config, but of course doing so does mean that it won't actually run against a default node any more. |
Sorry for not providing enough information to reproduce this: I used the metadata from the kilt network and the only change to the config was that I set the Dependencies look look like this:
|
@trusch that file does not contain the metadata, just the |
@ascjones My bad, I was a bit in a hurry and I guess I accidentally overwrote that file. Here is the correct one: |
@trusch can you try now? |
@ascjones It worked! The particular error I experienced is gone now 👍 I still can't get it to fully compile because one of the types generated from the metadata doesn't properly implement
But nevertheless, thanks for picking up and solving my initial issue so fast! |
I'm guessing that is because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, left a few nits and doc suggestions.
codegen/src/api/mod.rs
Outdated
/// Most chains require a valid account nonce as part of the extrinsic, so the default behaviour of | ||
/// the client is to fetch the nonce for the current account (if it has not been specified). | ||
/// | ||
/// The account nonce is commonly stored in the `System` pallet's `Account` storage item. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's actually possible to build a chain that stores the nonce elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "nonce" here the same as the "Account index"? I think both terms are fine, but we should perhaps clarify that it's the Config::Index
associated type we're referring to here (if that's what it is).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's actually possible to build a chain that stores the nonce elsewhere?
It would involve a custom frame_system
pallet. It's also possible that this will change across versions of frame_system
, I know it has in the past
Is "nonce" here the same as the "Account index"?
Yes indeed it is used interchangeably. Personally I find index slightly misleading, and the field is called nonce, but I can change the assoc type name here for consistency?
.iter() | ||
.find(|f| f.name() == Some(&"nonce".to_string()))? | ||
} else { | ||
abort_call_site!("Expected a `nonce` field in the account info struct") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both this assumption and the one below about it being a map should be documented: "The macro expects the AccountInfo
type to be a map and have a nonce
field." or something along those lines.
type BlockNumber = <DefaultConfig as Config>::BlockNumber; | ||
type Hash = <DefaultConfig as Config>::Hash; | ||
type Hashing = <DefaultConfig as Config>::Hashing; | ||
type AccountId = <DefaultConfig as Config>::AccountId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this example would be more useful if we made a more plausible change to the config, something a project might actually want to do. Not for this PR, it'd be more like a tutorial I reckon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would be best accompanied with an actual runtime with the different type
Co-authored-by: David <dvdplm@gmail.com>
Co-authored-by: David <dvdplm@gmail.com>
* Add custom config example * Make account data storage item configurable * Fix contracts * Regenerate polkadot codegen * Fmt * Specify different type for `MyConfig::Index` * Update comment in custom config example * Assign concrete types for default AccountData impl * Fmt * Fix contracts tests * Fmt * Add comments * Unlink doc comment trait (subxt not in scope) * Fix missing nonce field error message * Update codegen/src/api/mod.rs Co-authored-by: David <dvdplm@gmail.com> * Update examples/custom_config.rs Co-authored-by: David <dvdplm@gmail.com> * Rename Nonce assoc type to Index for consistency * Add module level docs about codegen assumptions Co-authored-by: David <dvdplm@gmail.com>
Fixes #391.
Previously the codegen hardcoded the
AccountData
parameter ofRuntimeApi
toDefaultAccountData
, which in turn was hardcoded to using theDefaultConfig
.This meant that the user was not able to supply their custom
Config
implemetnation, nor a customAccountData
implementation.This PR allows the user to supply their own custom
Config
impl, and optionally a customAccountData
impl.Update
The macro now autogenerates the
DefaultAccountData
impl against theSystem::Account
storage entry, and hardcodes the associated key (AccountId
) and value (Nonce
) types by looking them under the known names in the metadata.I really wanted to allow the type system to do as much as possible with no macro magic, but couldn't get it to work due to this error. So now what it does is it e.g. looks up the type of
System::Account::nonce
and assigns it totype Nonce
.