Closed SWvheerden closed 2 years ago
Should have also mentioned in the review that this approach technically reduces the effectiveness of the emoji ID. If a different network identifier is used during decoding and a transmission error occurs that mangles the emoji ID in the right way, it's possible for the checksum to pass when it shouldn't. This should probably be noted somewhere.
It might also be useful to indicate in the narrative why the network byte is used, and to highlight that addresses intended for different use cases should have different network bytes.
Further, if it's in scope, it might be helpful to specify which network bytes Tari will use, or at least which network differentiations will be needed (project, address type, network).
Just waiting for approval from @AaronFeickert, then can merge
I still think the encoding and algorithm descriptions aren't strictly correct as written (but maybe this is being too picky).
In the encoding algorithm, the step
Use the DammSum algorithm with `k = 8` and `m = 32` to compute an 8-bit masked checksum `'C` from `B` and network `N`.
isn't quite right, because DammSum doesn't produce masked checksums. Further, the algorithm doesn't specify that N
is an input to it.
In the decoding algorithm, it also isn't specified that N
is a required input.
The missing step in the encoding algorithm is to get the unmasked checksum from DammSum and then apply the masking. I'm a fan of making algorithm descriptions as clear as possible; if I were a first-time reader, I'd probably need to check an implementation to confirm what's actually going on (which reduces the effectiveness of the RFC).
To be specific, I'd change the encoding algorithm steps to be something like this:
The emoji ID is produced from a node public key `B` (serialized as 32 bytes) and a network identifier `N` (serialized as 8 bits) as follows:
- Use the DammSum algorithm with `k = 8` and `m = 32` to compute an 8-bit checksum `C` using `B` as input.
- Compute the masked checksum `C' = C XOR N`.
- Encode `B` into an emoji string using the emoji map.
- Encode `C'` into an emoji character using the emoji map.
- Concatenate `B` and `C'` as the emoji ID.
And I'd change the "preamble" of the decoding algorithm to be something like this:
The node public key is obtained from an emoji ID and a network identifier `N` (serialized a 8 bits) as follows:
Description
Move to stable Update the EmojiID to encode the network address using the checksum Do Grammarly check