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.

Sign up for GitHub

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

Jump to bottom

Polkadot Protocol > Protocol Components > Accounts > Address Formats #49

Merged
merged 18 commits into from
Oct 4, 2024
Merged

Polkadot Protocol > Protocol Components > Accounts > Address Formats #49

merged 18 commits into from
Oct 4, 2024

Conversation

kapetan3sid
Copy link
Collaborator

github-actions[bot]
github-actions bot reviewed Sep 23, 2024
View reviewed changes
polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
@kapetan3sid kapetan3sid self-assigned this Sep 23, 2024
@kapetan3sid kapetan3sid added the B0 - Needs Review Pull request is ready for review label Sep 23, 2024
@kapetan3sid kapetan3sid requested review from CrackTheCode016 and DrW3RK and removed request for albertov19 and dawnkelly09 September 24, 2024 18:10
@0xLucca
Copy link
Collaborator

0xLucca commented Oct 1, 2024

There is something strange in this PR, because you have like 30 modified files

0xLucca
0xLucca requested changes Oct 1, 2024
View reviewed changes
Copy link
Collaborator

@0xLucca 0xLucca left a comment

Choose a reason for hiding this comment

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

Some small fixes but is overall ok

polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
CrackTheCode016
CrackTheCode016 reviewed Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@CrackTheCode016 CrackTheCode016 left a comment

Choose a reason for hiding this comment

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

Overall looking good, I think we could add more links back to the SDK for those who are more curious, but it is mostly fine as-is

polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Show resolved Hide resolved
polkadot-protocol/protocol-components/accounts/address-formats.md Outdated

Polkadot SDK offers several potential checksum strategies, each providing varying length and longevity guarantees.

There are two types of checksum preimages (`SS58` and `AccountID`) along with a range of checksum lengths from 1 to 8 bytes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would nice to link these (SS58 and AccountID), unfortunately I couldn't find this right away in the codebase.. so not exactly sure what it is referring to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add this comment to the sheet and try to find it in code base, same result from my side. I can't find the right code to link here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure it is here, but I am not completely certain. Perhaps we can merge this, and find and add it later?

https://github.com/paritytech/polkadot-sdk/blob/3313163485c4d4946ea97477407107796469f1c4/substrate/primitives/core/src/crypto.rs#L277

polkadot-protocol/protocol-components/accounts/address-formats.md Outdated

The bytes used are always the left-most bytes. The input consists of the non-checksum portion of the SS58 byte series, which is used as input to the base-58 function.

For example, this input is formed by concatenating the `concat ( <address-type>, <address> )`. A context prefix of `0x53533538505245` (the string "SS58PRE") is then prepended to this input to create the final hashing preimage.
Copy link
Collaborator

Choose a reason for hiding this comment

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

For some of these functions, again, it would be nice to link them for clarity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ditto

polkadot-protocol/protocol-components/accounts/address-formats.md Outdated Show resolved Hide resolved
kapetan3sid and others added 5 commits October 3, 2024 16:11
@kapetan3sid
add subkey explanation
c965589
@kapetan3sid @0xLucca
update to -JS
7079124 
Co-authored-by: 0xLucca <95830307+0xLucca@users.noreply.github.com>
@kapetan3sid @0xLucca
update formatting
4708b58 
Co-authored-by: 0xLucca <95830307+0xLucca@users.noreply.github.com>
@kapetan3sid @0xLucca
update to -JS-2
7a67fa9 
Co-authored-by: 0xLucca <95830307+0xLucca@users.noreply.github.com>
@kapetan3sid @0xLucca
update to -JS-3
611c4c2 
Co-authored-by: 0xLucca <95830307+0xLucca@users.noreply.github.com>
@eshaben eshaben requested a review from a team as a code owner October 4, 2024 02:39
0xLucca
0xLucca approved these changes Oct 4, 2024
View reviewed changes
Copy link
Collaborator

@0xLucca 0xLucca left a comment

Choose a reason for hiding this comment

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

LGTM

@0xLucca 0xLucca requested a review from a team October 4, 2024 11:30
@eshaben eshaben merged commit bdb2772 into master Oct 4, 2024
@eshaben eshaben deleted the andreja/address-formats branch October 4, 2024 19:38
@0xLucca 0xLucca mentioned this pull request Oct 8, 2024
Closed
@dawnkelly09 dawnkelly09 mentioned this pull request Oct 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@0xLucca 0xLucca 0xLucca approved these changes

@eshaben eshaben eshaben approved these changes

@DrW3RK DrW3RK Awaiting requested review from DrW3RK

@dawnkelly09 dawnkelly09 Awaiting requested review from dawnkelly09

@CrackTheCode016 CrackTheCode016 Awaiting requested review from CrackTheCode016

Assignees

@kapetan3sid kapetan3sid

Labels
B0 - Needs Review Pull request is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kapetan3sid @0xLucca @eshaben @CrackTheCode016 @dawnkelly09