mobilecoinfoundation / mobilecoin

Private payments for mobile devices.
Other
1.16k stars 148 forks source link

serialize/deserialize u64 subaddress [index] as quoted decimal string #3996

Closed holtzman closed 3 months ago

holtzman commented 3 months ago

When serializing the TxoUnsynced struct, the subaddress member, which is a u64, should be represented as a string containing the decimal numeric representation of the u64, rather than as an unquoted decimal number directly, and when deserializing it, parse the subaddress as a string containing a decimal number.

Motivation

Currently, the subaddress member is being serialized as a number. This is unsafe in the face of parsers that do not allocate enough bits to parsing numbers, for example many JavaScript JSON parsers, resulting in loss of precision, and thereby alteration of the value, as rounding occurs.

This is an issue that we have encountered in general with serialized u64s, and specifically with the serializing of the TxoUnsynced struct. Our common practice has become to serialize u64s as strings containing the decimal numeric value of the u64 so as to be human readable while surviving intermediate parsing and processing intact, prior to be deserialized again.

github-actions[bot] commented 3 months ago

⚠️ Downstream repo mobilecoinofficial/full-service failed to build. Check actions status for details.

github-actions[bot] commented 3 months ago

⚠️ Downstream repo mobilecoinofficial/full-service failed to build. Check actions status for details.

holtzman commented 3 months ago

looks good. Would have been nice to have a sanity test or two showing the behavior, but it's on par with the rest of the file

Is the only other implementation of this logic in full service? I thought there would have been another implementation available in this repo

Thanks!

It is a bit anti-pattern for the serialization to be implemented in mobilecoin.git rather than full-service for the full-service use-case. However, this repo was was already implementing serialization for the other struct member, tx_out_public_key so it made sense to implement it here for subaddress as well.

The struct is used by other repos that submodule mobilecoin, as well.