-
Notifications
You must be signed in to change notification settings - Fork 335
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
Dedicated AccountId20
type
#926
Conversation
@JoshOrndorff Please use |
Or |
6b18f6f
to
1f9bc56
Compare
When we move this upstream into frontier, @tgmichel recommended we make a |
AccountId20
type
PartialOrd, | ||
Ord, | ||
)] | ||
pub struct AccountId20(pub [u8; 20]); |
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.
@joelamouche This is the type. The moonbase runtime's metadata contains this. So I guess the path is account::AccountId20
which makes sense from a Rust perspective.
{
id: 0
type: {
path: [
account
AccountId20
]
params: []
def: {
Composite: {
fields: [
{
name: null
type: 1
typeName: [u8; 20]
docs: []
}
]
}
}
docs: []
}
}
I'll let @nanocryk tell me how generic it is really appropriate to be here.
Thanks a lot for your help @girazoki 🙏 This is ready for your review and experimentation @joelamouche (or @crystalin because you mentioned that having the poor account formatting is making your testing procedures take longer). |
@joelamouche I believe I've done as you asked for the ts dependencies. Can you confirm? Feel free to revert those commits or push new commits if this is not right. |
WIP on a PR that will fix the tests |
* remove tolowercase * wip fixing tests * fix tests * finish fixing tests * remove todos * remove more todos * update package-lock
@JoshOrndorff pls look at the failing rust tests:
|
|
type errors due to polkadot-js/api#4213 for latest version and this old version is not up to date |
Problem with types is now tracked here: polkadot-js/api#4241 |
* tried downgrading * update types
* initial draft installed in moonbase * fix cli-opt * comment about better Display impl * `Copy`, `FromStr`, and make it build * rename to AccountId20 * Same FromStr error as AccountId32 * more comment ideas * Do Moonriver and Moonbeam runtimes * simplify asset_id_to_account * start on moonbase tests * moonbase tests pass * river and beam * proper blanket impl * Be generic over the destination type too 🤷 I'll let @nanocryk tell me how generic it is really appropriate to be here. * fix account tests * Fix tests * update @PolkaDot dependencies in types bundle * update @PolkaDot dependencies in tests * update @PolkaDot dependencies in tools * unused imports * update deps and fix tests (#975) * Joshy moonbeam account : remove toLowercase (#977) * remove tolowercase * wip fixing tests * fix tests * finish fixing tests * remove todos * remove more todos * update package-lock * Fix recently-merged integration tests * fix crossed dependencies in runtime-common * fix package-lock * downgrade polkadot/types because its broken * update deps * update deps * Joshy moonbeam account downgrade attempt (#1019) * tried downgrading * update types Co-authored-by: gorka <gorka.irazoki@gmail.com> Co-authored-by: Antoine Estienne <antoine@purestake.com> Co-authored-by: estienne.antoine@gmail.com <estienne.antoine@gmail.com>
What does it do?
This PR adds a dedicated type
AccountId20
rather than using the general purpose hash typeH160
as previously.What important points reviewers should know?
In general, this strengthens the type system's ability to prevent us from putting inappropriate data where an account id is expected. But speaking practically, we're hoping this will allow tools that depend on type metadata (like PolkadotJs) to better infer that account-related UI components should be used.
Is there something left for follow-up PRs?
Much of this can be upstreamed to Frontier and generalized. For now this is ready for PoC work on the Polkadot JS tools.
What value does it bring to the blockchain users?
Hopefully, nicer formatting of Account Ids in UI components and easier ability to choose accounts from a list (rather than pasting bytes).