Skip to content

call class serialize/deserialize methods#20

Merged
mehtaphysical merged 2 commits intomasterfrom
feat/add-ability-to-serialize-on-class
May 27, 2021
Merged

call class serialize/deserialize methods#20
mehtaphysical merged 2 commits intomasterfrom
feat/add-ability-to-serialize-on-class

Conversation

@mehtaphysical
Copy link
Contributor

@mehtaphysical mehtaphysical commented May 20, 2021

Fixes #19

This allows for something like:

export class PublicKey extends Assignable {
    keyType: KeyType;
    data: Uint8Array;

    serialize(writer: BinaryWriter) {
        writer.writeU8(this.keyType);
        writer.writeFixedArray(this.data);
    }

    static deserialize(reader: BinaryReader): PublicKey {
        const keyType = reader.readU8();
        const size = KeyType.ED25519 ? 32 : 65;
        return new PublicKey({ keyType, data: reader.readFixedArray(size) });
    }
}

"dependencies": {
"@types/bn.js": "^4.11.5",
"bn.js": "^5.0.0",
"bn.js": "^5.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

nice cleanup!

lib/index.js Outdated
}
function serializeStruct(schema, obj, writer) {
if (typeof obj.serialize === 'function') {
obj.serialize(writer);
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer obj.borshSerialize/obj.borshDeserialize for a less chance of name collision

Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

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

Code change looks good to me, @volovyk-s do you think the API change makes sense?

Copy link
Contributor

@volovyks volovyks left a comment

Choose a reason for hiding this comment

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

Looks good overall. Let's add simple unit tests and merge this PR.

@mehtaphysical mehtaphysical requested a review from volovyks May 26, 2021 00:59
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.

Custom Serialize/Deserialize

3 participants