-
Notifications
You must be signed in to change notification settings - Fork 217
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
Address Format in Shelley (Part 2: implementing address encoder and decoders + tests) #336
Conversation
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.
lgtm
-- in order to figure out to which class the address belongs. | ||
instance EncodeAddress Jormungandr where | ||
encodeAddress _ (Address bytes) = do | ||
if isJust (decodeLegacyAddress bytes) then base58 else bech32 |
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.
💯
("Unable to decode address: encoding is neither Bech32 nor Base58.") | ||
|
||
-- NOTE: | ||
-- Data belows have been generated with [jcli](https://github.com/input-output-hk/jormungandr/tree/master/doc/jcli) |
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.
minor, singular: data below
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.
Fixed 👍
let prefix = BS.pack | ||
[ 130 -- Array(2) | ||
, 216, 24 -- Tag 24 | ||
, 88, fromIntegral n -- Bytes(n), n > 23 && n < 256 |
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.
what if n
is choosen out of the range "n > 23 && n < 256" ?
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.
This will result in a non-valid CBOR encoding. 88
is a major type which indicates that the next item is a bytestring and the length of that bytestring is encoded in the next byte n
. So n
can't be greater than 255 (it's a byte), and, in cbor, values lesser than 23
are encoded directly in the payload of the major type and thus, there would be no n
.
This is how most addresses are represented in Byron, although the address payload is malleable so it could be bigger than 255 bytes in practice; but it doesn't really matter much for the test here.
, 88, fromIntegral n -- Bytes(n), n > 23 && n < 256 | ||
] | ||
payload <- BS.pack <$> vectorOf n arbitrary | ||
let crc = BS.pack [26,1,2,3,4] |
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.
I guess we don't compute crc from actuall payload here - maybe its not important for this test case
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.
Indeed. The crc is encoded using 32-bit, so, in some cases, it may be shorter than 4 bytes in rare cases. Here however, since the major type is 26
, it indicates an unsigned integer encoded over 4 bytes. This is really just about getting the structure right, not really providing any relevant pieces of information.
Generating Golden Test Vectors | ||
-------------------------------------------------------------------------------} | ||
|
||
-- SPENDINGKEY=$(jcli key generate --type Ed25519Extended | jcli key to-public) |
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.
this is useful to have here
1220882
to
ae69395
Compare
case BS.length bytes of | ||
33 -> when (BS.take 1 bytes /= BS.pack [single]) $ | ||
Left (invalidFirstByte single) | ||
65 -> when (BS.take 1 bytes /= BS.pack [grouped]) $ |
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.
What is special about the constants 33
and 65
? Can they be given names (and a comment), instead of hard-coding them?
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.
1 discriminant byte + 32 bytes of spending key (+ 32 bytes of delegation key for grouped address).
Named variables would be better indeed 👍
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.
Fixed.
|
||
-- NOTE: | ||
-- Data below have been generated with [jcli](https://github.com/input-output-hk/jormungandr/tree/master/doc/jcli) | ||
-- as described in the annexe at the end of the file. |
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.
Small typo: this should probably be annex
.
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.
Fixed.
<> T.singleton separatorChar | ||
<> T.pack dcp | ||
where | ||
dcp = dataCharFromWord <$> dataPartToWords dp <> createChecksum hrp dp |
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.
👍
@@ -293,27 +295,40 @@ humanReadableCharMaxBound = chr 126 | |||
Encoding & Decoding | |||
-------------------------------------------------------------------------------} | |||
|
|||
-- | Like 'encode' but allows output to be longer than 90 characters. This isn't | |||
-- ideal as the error detection becomes worse as string get longer but still | |||
-- acceptable. |
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.
Tiny typo in comment. This should probably read:
"as the string gets longer, but it's still acceptable."
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.
Fixed.
ae69395
to
5369138
Compare
Issue Number
#239
Overview
bech32
encode, decode and hrp smart-constructorsbech32
. By default, the encoder and decoder fail with a "StringTooLong" error if the end-result is > 90 chars because this is the optimal size for this encoding. Nevertheless, the encoding is fine up to 1024 chars and grouped addresses actually generate addresses that are longer than 90 chars!EncodeAddress
andDecodeAddress
forJormungandr
jcli
) to make sure everything's okay. Everything is :)Comments