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

Altered json to be more regular #8

Merged
merged 29 commits into from
Oct 4, 2021
Merged

Altered json to be more regular #8

merged 29 commits into from
Oct 4, 2021

Conversation

gilescope
Copy link
Contributor

@gilescope gilescope commented Sep 14, 2021

Less exceptions:

  • "symbols": [], rather than null.
  • network not optional. Now set for bare accounts: "network": "BareEd25519",
  • standardAccount = null implies Reserved.

(See also paritytech/substrate#9755 )

ss58-registry-derive/Cargo.toml Outdated Show resolved Hide resolved
ss58-registry-derive/src/lib.rs Outdated Show resolved Hide resolved
ss58-registry-derive/src/lib.rs Outdated Show resolved Hide resolved
ss58-registry-derive/src/lib.rs Outdated Show resolved Hide resolved
ss58-registry-derive/src/lib.rs Outdated Show resolved Hide resolved
ss58-registry-derive/src/lib.rs Outdated Show resolved Hide resolved
gilescope and others added 5 commits September 14, 2021 14:37
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
src/lib.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
No way to have two instances of Ss58AddressFormat that mean the same thing but are different.
(Before Custom(42) was the same as Ss58AddressFormat::Substrate but not eq.)
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
gilescope and others added 8 commits September 28, 2021 11:44
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
(brought in from master merge on substrate PR)
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Just some tests would be nice.

src/lib.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Sep 29, 2021

Looks good.

Modulo the nitpicks.

build.rs Outdated
let mut registry = registry.registry;

let mut ordered_prefixes = registry.iter().map(|i| i.prefix).collect::<Vec<_>>();
ordered_prefixes.sort_unstable();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use sort_unstable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's sorting primitives - there's no difference between 6 and 6 in this case - it's just a tad faster. In general I agree we don't want non-determinism but here I believe it makes no difference which we use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use sort().

it's just a tad faster.

We are sorting here like in maximum 256 elements, no one will be able to tell the difference.

build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
benches/benches.rs Show resolved Hide resolved
gilescope and others added 6 commits October 1, 2021 05:45
@gilescope gilescope merged commit d03a06f into main Oct 4, 2021
@gilescope gilescope deleted the vNext branch March 18, 2022 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants