Encode data using base64, encode ids using Hex for human readable serialisation #1304

patrickkuo · 2022-04-09T10:19:34Z

This PR created 2 custom de/serialiser BytesOrHex, BytesOrBase64 for : SuiAddress, ObjectDigest, PublicKeyBytes, Signature, TransactionDigest, and AuthoritySignature, data will be serialised as Bytes for non-human readable serialiser and encoded with base64/Hex for human readable serialiser.

Here are the json output of a transfer transaction after the changes:

patrick@Patricks-MBPMax release % ./wallet transfer --to 66595B3DE05980BE6CEE212EF55BB027AFDDF6C3 --object-id 388AEE712653AC793B04B00644CC47A9C95AEC89 --gas 54886B969591DA3ED8E4E95E2343EB8A4303C9F0 --gas-budget 1000 --json --no-shell | json_pp
[
   {
      "signatures" : [
         [
            "ixE/aWAYHYJouMulwi3VkvMX6WyVhaVfUwPntfjFoGw=",
            "Hir4XDhlLSWIstOOKil1xEt878eBhX9qcoctGYKsrFK+kpJYuaWzGvuVV0hfJEI9xgZt68FP5nQ9zhti+ZFNBQ=="
         ],
         [
            "toYwLGGvZ6mbI/WNDG0WHQskZ8ErTg2sJzToPPT9rZc=",
            "7oqmIBm+Mtdc8WqaCRyY6D3SvqunpO5XyVZjma9yFQBvhAzZX7VOW3VzrhmAMA42vPAf+xpX5RGmEZfUdvWbBg=="
         ],
         [
            "kgxikutUsshbqcwR/r9kfTw9sjgbLN2dv0MgA+51iuY=",
            "IqpNUB2CgMJja+iVBgUyrnmrxtbxm8gcDxVwLGtIkT8Mxo1PZUOEpLA5Nr6VJUEXmvij7Df8vw/01YfSD0kmCw=="
         ]
      ],
      "transaction" : {
         "auth_signature" : {},
         "data" : {
            "gas_budget" : 1000,
            "gas_payment" : [
               "54886b969591da3ed8e4e95e2343eb8a4303c9f0",
               3,
               "Ad7N808R/tCoI/MJBe985uxbLmWw0ewQgIh1MnhPEvE="
            ],
            "kind" : {
               "Single" : {
                  "Transfer" : {
                     "object_ref" : [
                        "388aee712653ac793b04b00644cc47a9c95aec89",
                        3,
                        "NoOzanRSFJ3WirXdBfb5IsoyW2qSHbX7vNelo6RuHCQ="
                     ],
                     "recipient" : "66595b3de05980be6cee212ef55bb027afddf6c3"
                  }
               }
            },
            "sender" : "66595b3de05980be6cee212ef55bb027afddf6c3"
         },
         "tx_signature" : "1pgheoIOqhUNcXVvAjBycSD2dOoI2e+5GtAXwHI+2UlAIu0QFJOm/pHsj3elA5ifNBO6HRXyezffXiTOWVCVDyAkrab7imXC+eeNo4ng92YxAmvNZtBOWZJhc/tn1hYF"
      }
   },
   {
      "created" : [],
      "deleted" : [],
      "dependencies" : [
         "txeL/KsZasQkUVG2/iCdsiqRr0JzIILUL7Ql32qYPVo="
      ],
      "events" : [],
      "gas_object" : [
         [
            "54886b969591da3ed8e4e95e2343eb8a4303c9f0",
            4,
            "EUGTXkztRb2/r35kXWibe267HQnsDZqiUTxiZXitKVA="
         ],
         {
            "AddressOwner" : "66595b3de05980be6cee212ef55bb027afddf6c3"
         }
      ],
      "mutated" : [
         [
            [
               "388aee712653ac793b04b00644cc47a9c95aec89",
               4,
               "Bwv3XBe2ctx3pGJWcKP6HBM0XEDRl5seQQbPPqeBDD0="
            ],
            {
               "AddressOwner" : "66595b3de05980be6cee212ef55bb027afddf6c3"
            }
         ],
         [
            [
               "54886b969591da3ed8e4e95e2343eb8a4303c9f0",
               4,
               "EUGTXkztRb2/r35kXWibe267HQnsDZqiUTxiZXitKVA="
            ],
            {
               "AddressOwner" : "66595b3de05980be6cee212ef55bb027afddf6c3"
            }
         ]
      ],
      "status" : {
         "Success" : {
            "gas_cost" : {
               "computation_cost" : 17,
               "storage_cost" : 0,
               "storage_rebate" : 0
            },
            "results" : []
         }
      },
      "transaction_digest" : "68+AJwrDCS/ELpsOyRVdCF1hW5xLsSv9CoxSp02h3xE=",
      "unwrapped" : [],
      "wrapped" : []
   }
]

codecov · 2022-04-09T10:48:44Z

Codecov Report

Merging #1304 (db3b2e1) into main (e615cda) will increase coverage by 0.24%.
The diff coverage is 93.90%.

❗ Current head db3b2e1 differs from pull request most recent head a5f5385. Consider uploading reports for the commit a5f5385 to get more accurate results

@@            Coverage Diff             @@
##             main    #1304      +/-   ##
==========================================
+ Coverage   81.70%   81.94%   +0.24%     
==========================================
  Files          99      100       +1     
  Lines       20766    20927     +161     
==========================================
+ Hits        16966    17148     +182     
+ Misses       3800     3779      -21

Impacted Files	Coverage Δ
sui/src/wallet.rs	`0.98% <0.00%> (ø)`
sui/src/wallet_commands.rs	`83.00% <0.00%> (-0.22%)`	⬇️
sui_types/src/base_types.rs	`77.74% <ø> (ø)`
sui_types/src/crypto.rs	`85.01% <ø> (+0.97%)`	⬆️
sui_types/src/lib.rs	`16.66% <ø> (ø)`
sui_types/src/readable_serde.rs	`92.55% <92.55%> (ø)`
sui_types/src/unit_tests/base_types_tests.rs	`100.00% <100.00%> (ø)`
...ammability/adapter/src/unit_tests/adapter_tests.rs	`98.55% <0.00%> (+0.10%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 777bf73...a5f5385. Read the comment docs.

awelc · 2022-04-11T01:30:49Z

Added @Clay-Mysten to track required doc changes.

bmwill · 2022-04-11T04:34:39Z

sui_types/src/readable_serde.rs

+        let s = String::deserialize(deserializer)?;
+        encoding
+            .decode(s)
+            .map_err(|_| D::Error::custom("byte deserialization failed"))


Whats the reason for throwing away the error from decoding, wouldn't it be useful to include that in the error message (via the error's Display impl) so that a user can get a better idea of why the deserialization failed? Eg from this error I don't know why it failed, did i give it hex when it wanted base64 or vice versa

👍thanks for pointing this out, I will add more info to the error

Clay-Mysten · 2022-04-11T15:05:42Z

Added @Clay-Mysten to track required doc changes.

I edited the code comments in readable_serde.rs directly. I assume you mean this will affect a bunch of examples/output in our existing docs.

Let's talk first when done with this then so I do the right thing. Hit me up in chat.

666lcz

Great work! Is it accurate to say everything will be serialized/deserialized using Base64 except SuiAddress?

666lcz · 2022-04-11T16:56:39Z

sui_types/src/readable_serde.rs

+byte_or_hex!(20,);
+byte_or_base64!(32, 96,);


nit: can we use Constants(e.g., OBJECT_DIGEST_LENGTH) from other files to improve readability?

666lcz · 2022-04-11T17:07:38Z

sui_types/src/unit_tests/base_types_tests.rs

@@ -169,6 +169,80 @@ fn test_object_id_serde_json() {
    assert_eq!(obj_id, json_obj_id);
 }

+#[test]
+fn test_object_id_serde_not_human_readable() {


Currently object id is passed in the rest server in hex:

sui/sui/src/rest_gateway/requests.rs

Line 57 in 7c30c0a

/// Required; Hex code as string representing Move module location

. Are we changing this behavior?

I am not changing ObjectID in this PR, ObjectID already have it's own serialiser implemented earlier and it's doing similar things : Hex for human-readable, and tuple array (which doesn't contain array length compare to Bytes) for non-human-readable serialisation, we might want to do a refactoring in the future though to make them consistent.

patrickkuo · 2022-04-11T17:18:48Z

Great work! Is it accurate to say everything will be serialized/deserialized using Base64 except SuiAddress?

Thanks!

The rules is Hex for IDs, so we can preserve the ordering, and base64 for data/signature/digest
so SuiAddress and ObjectID will be encoded to Hex

velvia · 2022-04-11T17:57:13Z

@patrickkuo hey this is a great PR, it does allow a way to serialize to human readable and bytes at the same time. The only ting I don't like, and this is not meant to be a discouragement, is that the API of is_human_readable really isn't very good, because it only offers ONE choice of what "human readable" means. What if we want to support, for example (this is I know a bit contrived, but) both base64 and base58? It seems over time this might become more and more common.

@bmwill I think implementing base64 and hex as specific methods to_base64 to_hex etc is much more flexible than shoehorning this into the SerDe mechanism. Base64 and hex aren't so much "serializations" as just different representations.

…zation

Serialise to bytes instead of tuplearray

Hyphenate compound modifiers, remove caps where not needed

patrickkuo · 2022-04-11T18:33:52Z

@patrickkuo hey this is a great PR, it does allow a way to serialize to human readable and bytes at the same time. The only ting I don't like, and this is not meant to be a discouragement, is that the API of is_human_readable really isn't very good, because it only offers ONE choice of what "human readable" means. What if we want to support, for example (this is I know a bit contrived, but) both base64 and base58? It seems over time this might become more and more common.

@bmwill I think implementing base64 and hex as specific methods to_base64 to_hex etc is much more flexible than shoehorning this into the SerDe mechanism. Base64 and hex aren't so much "serializations" as just different representations.

One reason of shoehorning the encoding to serde is to avoid having to do the conversion manually in the RPC layer; we can achieve the same result by create a different set of all the return types and convert them in the RPC layer, but that's quite a bit of work as our return objects are quite complex.

velvia · 2022-04-11T18:45:10Z

@patrickkuo

One reason of shoehorning the encoding to serde is to avoid having to do the conversion manually in the RPC layer

Would you mind pointing out where the RPC code is? Just want to have a look and see how much impact this would be...

patrickkuo · 2022-04-11T18:54:26Z

@patrickkuo

One reason of shoehorning the encoding to serde is to avoid having to do the conversion manually in the RPC layer

Would you mind pointing out where the RPC code is? Just want to have a look and see how much impact this would be...

For example in execute_transaction function, we will need to handle/convert the TransactionResponse manually to JSON without the original serde.
https://github.com/MystenLabs/sui/blob/35d7ce0352531bd873a929036db8d0f71c90fd78/sui/src/rpc_gateway.rs#L92-L98

Also for other functions we will need to convert the input SuiAddress/ObjectID manually.

huitseeker

Ack on direction, I pointed at a couple issues in the PR.

sui_types/src/readable_serde.rs

huitseeker · 2022-04-12T12:08:50Z

sui_types/src/readable_serde.rs

+            Encoding::Base64 => base64::decode(s)?,
+            Encoding::Hex => hex::decode(s)?,
+        })
+    }
+
+    fn encode<T: AsRef<[u8]>>(&self, data: T) -> String {
+        match self {
+            Encoding::Base64 => base64::encode(data),


There are issues with the base64 crate in cryptographic contexts (I don't think you're serializing private keys, but you're serializing public ones at least). Please make sure you contact @kchalkias for advice.
base64ct does not have those issues, you may want to use it instead.

I have replaced base64 with base64ct

* replace base64 with base64ct

Clay-Mysten · 2022-04-12T22:57:44Z

@awelc and @patrickkuo Please help me understand the docs needing updating as a result of this change.

I am happy to take it from there. Thanks!

patrickkuo · 2022-04-13T16:38:06Z

The changes here mainly impacting the JSON output, I had a quick look it seems like we do not include JSON output in our docs? @Clay-Mysten

Clay-Mysten · 2022-04-13T17:05:47Z

The changes here mainly impacting the JSON output, I had a quick look it seems like we do not include JSON output in our docs? @Clay-Mysten

What about here?
https://docs.sui.io/build/rest-api#get-apiobjects
https://docs.sui.io/build/rest-api#post-apipublish

Thanks, Patrick!

Clay-Mysten · 2022-04-14T15:23:43Z

@awelc for guidance on whether these need updates:
https://docs.sui.io/build/rest-api#get-apiobjects
https://docs.sui.io/build/rest-api#post-apipublish

patrickkuo · 2022-04-14T16:57:43Z

@Clay-Mysten sorry for late reply, I skimmed through the docs and seems like we didn't include JSON output samples in them, so nothing to update.... @awelc to confirm

Clay-Mysten · 2022-04-19T23:37:07Z

No worries, @patrickkuo! Thanks for double checking.

patrickkuo self-assigned this Apr 9, 2022

patrickkuo requested review from huitseeker, gdanezis, lxfind, 666lcz, arun-koshy, stella3d and velvia April 9, 2022 10:21

patrickkuo force-pushed the pat/human_readable_serialization branch 2 times, most recently from c5dc349 to d5eb5cf Compare April 10, 2022 15:27

patrickkuo linked an issue Apr 11, 2022 that may be closed by this pull request

[wallet CLI] Display Owner and TransactionDigest as hex #1306

Closed

patrickkuo mentioned this pull request Apr 11, 2022

[wallet CLI] Display Owner and TransactionDigest as hex #1306

Closed

awelc requested a review from Clay-Mysten April 11, 2022 01:30

bmwill reviewed Apr 11, 2022

View reviewed changes

patrickkuo force-pushed the pat/human_readable_serialization branch from cc68395 to 89dc1ac Compare April 11, 2022 10:33

patrickkuo requested a review from oxade April 11, 2022 11:37

666lcz approved these changes Apr 11, 2022

View reviewed changes

patrickkuo and others added 6 commits April 11, 2022 19:16

serialize data using base64, ids using Hex for human readable seriali…

00ec57b

…zation

BytesOrHex, BytesOrBase64 refactoring

df1192c

Serialise to bytes instead of tuplearray

Use default serde for AuthoritySignature instead of Bytes

9435404

add more info to error

f6cadff

Update readable_serde.rs

eaaf7aa

Hyphenate compound modifiers, remove caps where not needed

address PR comments

db3b2e1

patrickkuo force-pushed the pat/human_readable_serialization branch from 66695fe to db3b2e1 Compare April 11, 2022 18:22

huitseeker reviewed Apr 12, 2022

View reviewed changes

* use const generic instead of macro

a5f5385

* replace base64 with base64ct

huitseeker approved these changes Apr 12, 2022

View reviewed changes

patrickkuo merged commit f7aa79c into main Apr 12, 2022

patrickkuo deleted the pat/human_readable_serialization branch April 12, 2022 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encode data using base64, encode ids using Hex for human readable serialisation #1304

Encode data using base64, encode ids using Hex for human readable serialisation #1304

patrickkuo commented Apr 9, 2022 •

edited

Loading

codecov bot commented Apr 9, 2022 •

edited

Loading

awelc commented Apr 11, 2022

bmwill Apr 11, 2022

patrickkuo Apr 11, 2022

Clay-Mysten commented Apr 11, 2022

666lcz left a comment

666lcz Apr 11, 2022

666lcz Apr 11, 2022

patrickkuo Apr 11, 2022

patrickkuo commented Apr 11, 2022

velvia commented Apr 11, 2022

patrickkuo commented Apr 11, 2022

velvia commented Apr 11, 2022

patrickkuo commented Apr 11, 2022

huitseeker left a comment

huitseeker Apr 12, 2022

patrickkuo Apr 12, 2022

Clay-Mysten commented Apr 12, 2022

patrickkuo commented Apr 13, 2022 •

edited

Loading

Clay-Mysten commented Apr 13, 2022

Clay-Mysten commented Apr 14, 2022

patrickkuo commented Apr 14, 2022 •

edited

Loading

Clay-Mysten commented Apr 19, 2022

		byte_or_hex!(20,);
		byte_or_base64!(32, 96,);

Encode data using base64, encode ids using Hex for human readable serialisation #1304

Encode data using base64, encode ids using Hex for human readable serialisation #1304

Conversation

patrickkuo commented Apr 9, 2022 • edited Loading

codecov bot commented Apr 9, 2022 • edited Loading

Codecov Report

awelc commented Apr 11, 2022

bmwill Apr 11, 2022

Choose a reason for hiding this comment

patrickkuo Apr 11, 2022

Choose a reason for hiding this comment

Clay-Mysten commented Apr 11, 2022

666lcz left a comment

Choose a reason for hiding this comment

666lcz Apr 11, 2022

Choose a reason for hiding this comment

666lcz Apr 11, 2022

Choose a reason for hiding this comment

patrickkuo Apr 11, 2022

Choose a reason for hiding this comment

patrickkuo commented Apr 11, 2022

velvia commented Apr 11, 2022

patrickkuo commented Apr 11, 2022

velvia commented Apr 11, 2022

patrickkuo commented Apr 11, 2022

huitseeker left a comment

Choose a reason for hiding this comment

huitseeker Apr 12, 2022

Choose a reason for hiding this comment

patrickkuo Apr 12, 2022

Choose a reason for hiding this comment

Clay-Mysten commented Apr 12, 2022

patrickkuo commented Apr 13, 2022 • edited Loading

Clay-Mysten commented Apr 13, 2022

Clay-Mysten commented Apr 14, 2022

patrickkuo commented Apr 14, 2022 • edited Loading

Clay-Mysten commented Apr 19, 2022

patrickkuo commented Apr 9, 2022 •

edited

Loading

codecov bot commented Apr 9, 2022 •

edited

Loading

patrickkuo commented Apr 13, 2022 •

edited

Loading

patrickkuo commented Apr 14, 2022 •

edited

Loading