-
Notifications
You must be signed in to change notification settings - Fork 125
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
Clean up trait bounds #541
Conversation
4d228f6
to
fa214de
Compare
type Index = u32; | ||
type Hash = H256; | ||
type AccountId = AccountId32; | ||
type Address = MultiAddress<Self::AccountId, 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.
AccountIndex and BlockNumber types are now a little hidden. I don't really see the benefit of not defiing these types.
E.g. add the types
type AccountIndex = u32;
type BlockNumber = u32;
And then assigned Header and Address with Self::BlockNumber / AccountIndex
?
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.
- BlockNumber was removed in subxt: See: Remove unneeded Config bounds and BlockNumber associated type paritytech/subxt#804. Matter of taste. But, if we add it to the config, it simplifies our code like above.
- AccountIndex is not used in substrate-api-client, has no traits and was removed from node-template : Remove Unused
AccountIndex
paritytech/substrate#9149. So I don't see the benefit to add it.
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.
Agreed with the AccountIndex. Regarding the BlockNumber I guess it's your choice. I think it makes it more complex than it helps, subxt thinks otherwise. Up to you :)
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 introduced the BlockNumber, to simplify our code. As this is the goal.
Sorry, I was so immersed in the code, that I totally forgot to mention: Thanks a lot for this PR! I think it's a huge improvement! 🥳 |
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.
Nice ! I really like it 🥳 Only small change requests, other than that I approve 👍
One thing: Can the Pallet Configs be removed now? E.g |
FrameSystemConfig is still used by
|
- Remove ExtrinsicParams from API. Add them to Config. - Remove Runtime from API:FrameSystemConfig,BalancesConfig,ContractsConfig - Add AccountData in primitives type - Add Block to API, as this can't be easily configured (requires the Call from the runtime) - Add config trait similar to subxt and a specific config for substrate node - Get the block number from Header Remove Signer Add StakingBalance in config Add a polkadot config Example and test compile and run succesffully Move Blocktype from API into config Clean examples: all use kitchensing runtime Clean code move config into own folder Clean after merge Clippy Remove unnecessary bounds Header::Number must implement copy Changes from review Changes from review Remove Signer from ExtrinsicSigner Changes from Review: - add type BlockNumber - add trait file and move hasher and Header traits there Changes from Review: make config generic over Call and use a own SubstrateBlock type - Define a specific SubstrateExtrinsic type - Implement Extrinsic,Serialize and Deserialize for UncheckedExtrinsicV4 - add unit tests Use of OpaqueExtrinsic: no need of Call input in Config. Add own OpaqueExtrinsic for no-std serialization
I removed the dependencies and deleted the pallet configs |
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.
Cool! Very nice. Only one comment left :)
primitives/src/traits.rs
Outdated
fn hash(&self) -> <Self::Hasher as Hasher>::Output { | ||
<<Self::Header as Header>::Hasher>::hash_of(self.header()) | ||
} | ||
//Self::Hash { |
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 this still needed?
{ | ||
type ExtrinsicAddress<T> = | ||
<<T as Config>::ExtrinsicSigner as SignExtrinsic<<T as Config>::AccountId>>::ExtrinsicAddress; | ||
type Signature<T> = |
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, that's a pity. But yeah, makes sense. Leave as is then
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.
Awesome, thanks a lot 🥳
Fixes: #406
The different types that are important to communicate with a specific node can be configured.
All examples use the substrate kitchensink runtime: the config is a substrate config with the kitchensink runtime block type
Issue #504 is obsolete. The ExtrinsicSigner does no more depend on Runtime input but from the configuration. It is cloneable. The StaticExtrinsicSigner is still available but instead of StaticExtrinsicSigner, we can use ExtrinsicSigner::<SubstrateConfig, sr25519::Pair>.