Skip to content
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

[TS SDK] Add the ability to serialize nested vectors #9635

Closed
wants to merge 3 commits into from

Conversation

xbtmatt
Copy link
Contributor

@xbtmatt xbtmatt commented Aug 12, 2023

Description

This PR exports the serializeVector function in builder_utils.ts and adds the ability to serialize nested vectors by recursively calling itself if the first element is also an array.

New:

  if (argVal[0] instanceof Array) {
    // If we are serializing a vector of vectors, we merely serialize the length of the outer vectors and then continue traversing deeper
    argVal.forEach((arg) => serializeVector(arg, argType, serializer, depth + 1));
  } else {
    // When the innermost element is reached, we finally serialize it.
    argVal.forEach((arg) => serializeArgInner(arg, argType.value, serializer, depth + 1));
  }

Old:

argVal.forEach((arg) => serializeArgInner(arg, argType.value, serializer, depth + 1));

I also exposed a helper function to do this, since this type of vector serialization wasn't exposed before, only used for automatic serialization with the ABIs.

So now you can do this:

    const ssv = BCS.smartSerializeVector([[0, 1, 2], [1, 2, 3], [2, 3, 4]], new TypeTagU8()));
    console.log(ssv);

    Uint8Array(13) [
      3, 3, 0, 1, 2, 3,
      1, 2, 3, 3, 2, 3,
      4
    ]

This doesn't have to go in the v1 SDK but wanted to get the idea of how to do it out there.

Edit: Will fix the dependency cycle soon

Test Plan

Added a mostly exhaustive unit test in helper.test.ts

I serialized the values in Move and then printed them out as hex strings to use in the unit tests.

Note that before, serializeVector(...) with nested vectors just threw an error, but now it won't.

import { Deserializer } from "./deserializer";
import { Serializer } from "./serializer";
import { AnyNumber, Bytes, Seq, Uint16, Uint32, Uint8 } from "./types";
import { serializeVector as smartSerializeVectorWithDepth } from "../transaction_builder/builder_utils";
Copy link
Contributor

Choose a reason for hiding this comment

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

lets import serializeVector as is, and call the helper function serializeVectorWithDepth that just calls serializeVector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok can do, but there's still a namespace collision bc there's a serializeVector function in the helper.ts file. I'll just import with serializeVector as serializeVectorHelper, lmk if you want it to be named something else

And some variable names
Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

what problem are we trying to solve? this seems like a very specific use case that doesn't apply to any existing code or examples?

@xbtmatt
Copy link
Contributor Author

xbtmatt commented Aug 14, 2023

what problem are we trying to solve? this seems like a very specific use case that doesn't apply to any existing code or examples?

Well I suppose I should have just left it as a draft to figure out where it would go first, but it was more of an idea on how we could handle nested vector serialization more robustly, but after discussing it with Maayan I've realized it probably makes more sense to offer fewer "helper" functions like this and just make the process of serialization clearer.

As for if anyone would really use it, I guess it is a fairly specific use case but I need it all the time for minting contracts when I'm uploading hundreds of property maps at a time😅

I can close the PR, I suppose it should have been in a feature request or discussion first

@xbtmatt xbtmatt marked this pull request as draft August 14, 2023 00:55
@xbtmatt xbtmatt changed the title [TS SDK] Added the ability to serialize nested vectors [TS SDK] Add the ability to serialize nested vectors Aug 14, 2023
@xbtmatt
Copy link
Contributor Author

xbtmatt commented Aug 14, 2023

I wrote this up earlier and wanted to outline the current issues with the SDK and serializing vectors. It's an example of how to currently serialize a boolean array

const serializer = new BCS.Serializer();
const bools = [1, 0, 1, 0, 0];

BCS.serializeVector

This function does not work as one might expect. It prepends the arg type to the beginning of each value. Used by the tx builder with ABIs, link here

const txArgBools = bools.map(b => new TxnBuilderTypes.TransactionArgumentBool(b));
BCS.serializeVector(txArgBools, serializer);
const bytes = serializer.getBytes();

// bytes
Uint8Array(11) [
    5, 5, 1, 5, 0, 5, 1, 5, 0, 5, 0
]

BCS.serializeVectorWithFunc

This function works, but the second argument is the serialization function denoted as a string.

  • The string names aren't consistent with the BCS.serializeX names. i.e. BCS.bcsSerializeBool, vs theSerializer class' serializeBool, which is what you have to use.
  • It also does not work with nested vectors.
bytes = BCS.serializeVectorWithFunc(bools, 'serializeBool');

// bytes
Uint8Array(6) [
    5, 1, 0, 1, 0, 0
]

Manual construction

This works, but is a pain because you must know already how to BCS serialize a vector yourself, specifically with the Serializer class

  1. Construct a new Serializer();
  2. Serialize the vector length as a uleb128
  3. Serialize each individual value
  4. And then get the bytes with .getBytes()
bytes= (() => {
    const serializer = new BCS.Serializer();  // 1
    serializer.serializeU32AsUleb128(LENGTH); // 2
    bools.forEach( (b) => 
        serializer.serializeBool(b.value) // 3
    );
    return serializer.getBytes(); // 4
})();

// bytes
Uint8Array(6) [
    5, 1, 0, 1, 0, 0
]

@0xmaayan
Copy link
Contributor

0xmaayan commented Aug 14, 2023

@xbtmatt

for #1, when you mention "as one might expect", would you expect it to not use txn arguments?

for #2, I believe we can have this function accept a SerializerFunctions type instead of a string, something like

type SerializerFunctions =
  | "serializeStr"
  | "serializeBytes"
  | "serializeFixedBytes"
  | "serializeBool"
  | "serializeU8"
  | "serializeU16"
  | "serializeU32"
  | "serializeU64"
  | "serializeU128"
  | "serializeU256"
  | "serializeU32AsUleb128";

I think it makes more clear to me why we have this helper file in the first place. those functions are not really part of the serializer but helper functions for popular use cases. I wonder if they even need to be part of the Serializer class. The Serializer/Deserializer classes can potentially live in their own BCS package in the future and those serializeVector functions are just a wrapper to the underlying serialization the classes provide us.

@xbtmatt
Copy link
Contributor Author

xbtmatt commented Aug 14, 2023

@xbtmatt

for #1, when you mention "as one might expect", would you expect it to not use txn arguments?

for #2, I believe we can have this function accept a SerializerFunctions type instead of a string, something like

type SerializerFunctions =
  | "serializeStr"
  | "serializeBytes"
  | "serializeFixedBytes"
  | "serializeBool"
  | "serializeU8"
  | "serializeU16"
  | "serializeU32"
  | "serializeU64"
  | "serializeU128"
  | "serializeU256"
  | "serializeU32AsUleb128";

I think it makes more clear to me why we have this helper file in the first place. those functions are not really part of the serializer but helper functions for popular use cases. I wonder if they even need to be part of the Serializer class. The Serializer/Deserializer classes can potentially live in their own BCS package in the future and those serializeVector functions are just a wrapper to the underlying serialization the classes provide us.

Yeah I think the SerializerFunctions would definitely help with clarity for the time being. Ideally it'd just accept a Serializer function rather than a string, but I couldn't figure out how to get typescript to let me do this in a way that made sense.

The most sensible approach (to me) for a more robust serialization function in v2 is allowing the user to pass in a value: any and a TypeTag. The TypeTag can be an option, string, or vector. And then it just handles the rest

This is basically how serializeArg works but it isn't exported and you also have to know how to construct a StructTag if you want to do an option or object or string.

Imo It would make the most sense if TypeTag offered (without extra construction) every type of value you can send in to Move right now, which is all the primitives + option/object/string

As for #1 I'm gonna play around with it today and see if the issue is just that I'm using the txBuilderArgument, I forget if that was the issue or not

@xbtmatt xbtmatt closed this Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants