Add traits and remove excessive type definition in extrinsic helpers #434
Conversation
haerdib
added
F7-enhancement
haerdib
changed the title
src/extrinsic/mod.rs
Outdated
| pub trait AssignExtrinsicTypes { | ||
| type Address; | ||
| type Signature; | ||
| type SignedExtra; | ||
| } | ||
|
|
||
| impl<Signer, Client, Params, Runtime> AssignExtrinsicTypes for Api<Signer, Client, Params, Runtime> | ||
| where | ||
| Signer: SignExtrinsic<Runtime::AccountId>, | ||
| Runtime: FrameSystemConfig, | ||
| Params: ExtrinsicParams<Runtime::Index, Runtime::Hash>, | ||
| { | ||
| type Address = Signer::ExtrinsicAddress; | ||
| type Signature = Signer::Signature; | ||
| type SignedExtra = Params::SignedExtra; | ||
| } |
There was a problem hiding this comment.
I think we don't need this trait if we have the Extrinsic associated types as I described above.
There was a problem hiding this comment.
I don't see the correlation : This trait is just so that not every trait needs to redefine these three types. Nothing to do with the Extrinsic I think?
There was a problem hiding this comment.
Nvm - there is a correlation. Forget the comment above .
There was a problem hiding this comment.
I thought Extrinsic is mostly where we use them as the trait is called AssignExtrinsicTypes, but you can quite conveniently have an extrinsic associated type to assign those values.
If this re-assignment is needed beyond the scope of extrinsics, we should add these associated types to an overarching ApiTrait IMO, so that if you use the api, you always have it in scope and you don't need to import this extra trait.
There was a problem hiding this comment.
I removed it. If it's about the re-assignment, as you wrote, it should be done a level above.
src/extrinsic/staking.rs
Outdated
| use ac_primitives::{CallIndex, ExtrinsicParams, RewardDestination, SignExtrinsic, StakingConfig}; | ||
| use codec::{Compact, Decode, Encode}; | ||
|
|
||
| const MODULE: &str = "Staking"; |
There was a problem hiding this comment.
I am not sure if I agree with the plain Module name. I would prefer to name STAKING_MODULE, what was your reasoning?
There was a problem hiding this comment.
Redundancy. The file is already called staking.rs. Why would everything need a Staking prefix?
There was a problem hiding this comment.
Alright, I see now that it is private, so I think it is ok. However, module is just so overly generic so I am still a bit against it.
There was a problem hiding this comment.
Kay kay, I'll give in this time. As thanks for the solution above 😉
There was a problem hiding this comment.
Readded the name. But for the MODULE only. For the calls, I still think that's an overkill.
The aim of this PR is to remove the excessive type definitions for the extrinsic creation. Until now, whenever there have been changes in types (see an example) lots of manual changes needed to be done. To solve this, several traits are introduced in this PR.
Drawback:
Additionally, the following was changed:
common.rsbecause all the types were for a specific pallet anyway. So moved them there.BALANCES_TRANSFER->TRANSFERcloses #415