Skip to content
This repository has been archived by the owner on Sep 14, 2023. It is now read-only.

feat: improve ss58 encoding/decoding #751

Merged
merged 7 commits into from
Mar 17, 2023
Merged

Conversation

kratico
Copy link
Contributor

@kratico kratico commented Mar 15, 2023

Resolves #619

Add ss58 encoding and decoding for different payload and checksum lengths (see ss58-format).

Note: MultiAdress.Index use formats whose checksum length is 1 provided that the index is of type u8, u16, u32 or u64 (1, 2, 4 or 8 byte payload).

image

4: 1,
8: 1,
32: 2,
33: 2,
Copy link
Contributor Author

@kratico kratico Mar 15, 2023

Choose a reason for hiding this comment

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

@kratico kratico marked this pull request as ready for review March 15, 2023 16:19
@tjjfvi
Copy link
Contributor

tjjfvi commented Mar 15, 2023

Note: MultiAdress.Index use formats whose checksum length is 1 provided that the index is of type u8, u16, u32 or u64 (1, 2, 3 or 4 byte payload).

It would be 1/2/4/8 byte payload, no?

@kratico
Copy link
Contributor Author

kratico commented Mar 15, 2023

Note: MultiAdress.Index use formats whose checksum length is 1 provided that the index is of type u8, u16, u32 or u64 (1, 2, 3 or 4 byte payload).

It would be 1/2/4/8 byte payload, no?

Yes!

Copy link
Contributor

@tjjfvi tjjfvi left a comment

Choose a reason for hiding this comment

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

Could we get a test for every valid address length and every valid payload length to make sure they always round-trip correctly?

@kratico
Copy link
Contributor Author

kratico commented Mar 15, 2023

Could we get a test for every valid address length and every valid payload length to make sure they always round-trip correctly?

I could add decoding tests for the 16 formats.
But for encoding, I will need to change the encode(...) signature to accept the desired checksum or I can leave it as is.
@tjjfvi WDYT?

@kratico kratico force-pushed the feat/multiaddress-to-ss58 branch from 9a91a9b to ccd29ed Compare March 15, 2023 18:47
@kratico kratico force-pushed the feat/multiaddress-to-ss58 branch from 0bfd44d to 3fe6178 Compare March 16, 2023 17:55
@kratico kratico requested a review from tjjfvi March 16, 2023 17:55
crypto/ss58.ts Outdated Show resolved Hide resolved
crypto/ss58.ts Outdated Show resolved Hide resolved
crypto/ss58.ts Outdated Show resolved Hide resolved
@kratico kratico force-pushed the feat/multiaddress-to-ss58 branch from f097dca to b9aa433 Compare March 16, 2023 22:15
@kratico kratico added this pull request to the merge queue Mar 17, 2023
Merged via the queue into main with commit d33ab55 Mar 17, 2023
@kratico kratico deleted the feat/multiaddress-to-ss58 branch March 17, 2023 00:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add multiAddressToSs58
2 participants