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

feat: Add BigInt serialization #228

Merged
merged 14 commits into from
Sep 27, 2022
Merged

Conversation

petarvujovic98
Copy link
Contributor

Add custom serialization function in utils
Improve types in near-bindgen and use serialization function Add separate serialization for function returns
Add serialization to collections
Add serialization tests
Add serialization build and tests scripts

Addresses #172.

This is just a design I came up with that we can iterate on.

Add custom serialization function in utils
Improve types in near-bindgen and use serialization function
Add separate serialization for function returns
Add serialization to collections
Add serialization tests
Add serialization build and tests scripts
@@ -12,7 +12,7 @@ export class LookupMap<DataType> {

get(key: Bytes, options?: GetOptions<DataType>): DataType | null {
const storageKey = this.keyPrefix + key;
const value = JSON.parse(near.storageRead(storageKey));
const value = deserialize(near.storageRead(storageKey));
Copy link
Member

Choose a reason for hiding this comment

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

It's great to make serialize / desalinize general / independent of the collections! Since we have GetOptions, it's possibly a good idea to make it as part of GetOptions, and have current utils.serialize/utils.deserialize as the default option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ailisp I have covered this in the latest commits. Let's discuss the approach I took there. I also added a serializer and deserializer option to the NearBindgen decorator

Copy link
Member

Choose a reason for hiding this comment

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

That looks great!

src/utils.ts Outdated
if (typeof value === "bigint") {
return {
value: value.toString(),
[BIGINT_KEY]: BIGINT_BRAND,
Copy link
Member

Choose a reason for hiding this comment

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

This is very clever mechanism! A little extension based on your approach can become more general. We don't need both BIGINT_KEY and BIGINT_BRAND to indicate it's a bigint, so, maybe it can be TYPE_KEY and TYPE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ailisp Could you perhaps give an example or code snippet of what you are thinking about when you say more general?

Copy link
Member

Choose a reason for hiding this comment

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

For example, if later we also want to support serialize and deserialize Date, we can extend your serialize code for date:

return {
  value: value.toString(),
  [TYPE_KEY]: DATE_TYPE,
}

And in deserialization, we can always check TYPE_KEY, see what type it is and call bigint / date deserialization correspondingly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ailisp I added something similar along with an implementation of Date serialization and an accompanying test in this commit

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.

I like your solution, it solves the bigint serialization in a clean, idiomatic way. I leave some comments to extend your framework a little bit. /cc @volovyks

Add beforeEach and afterEach instead of before and after
Remove unused account import
Add serialization and deserialization to options
Add serialization and deserialization options to NearBindgen
Extract some common logic to utils
Refactor unordered-map types
Add exports to class declarations
Remove unused variables
Fix `any` types
Fix `moduleResolution` in examples
Format `jsconfig.json` `includes` entry for CLI
@petarvujovic98
Copy link
Contributor Author

@volovyks @ailisp We should change examples and tests to use the new serialization, and to use the options we added in #213.
Do we want to do this in this PR or create a separate PR?

Improve types for decorators and babel plugin
Improve performance of bindgen exporter and clean up code
Update FT example to remove redundant BigInt calls and use defaultValue
@ailisp
Copy link
Member

ailisp commented Sep 22, 2022

Do we want to do this in this PR or create a separate PR?

I think create a separate PR would be great!

@ailisp ailisp linked an issue Sep 23, 2022 that may be closed by this pull request
Generalize type branding
Add `Date` object serialization and tests
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.

Wow you're awesome! I was just mention Date as an imaginary example and you've already made it with ! It will be helpful too, developers will be glad to serialize and deserialize date automatically.

@volovyks If you also agree with the change I think we can merge it

};

return JSON.stringify(valueToSerialize, (_, value) => {
if (typeof value === "bigint") {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to check type isinstance of Date instead of override Date.prototype?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, by the time you get the value argument inside the stringifyreplacer, the toJSON function of the Dateobject is already called so I cannot infer the type of the variable, I tried several ways to make it happen but this was the only one that worked, evem though I don't like it's hacky nature/playing around with prototypes

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for clarification!

Copy link
Member

Choose a reason for hiding this comment

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

Also doesn't have luck with value - as you said it's already string, but I found this works: https://stackoverflow.com/a/54037861

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I just updated the Date serialization function with this suggestion and it seems to work as expected.

t.is(afterSet, newDate.toISOString());
});

test("get date field in milliseconds", async (t) => {
Copy link
Member

Choose a reason for hiding this comment

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

These tests are great 👍

Remove prototype override and add key based type checking
Add `validateAccountId` util
Remove unneccesary character escape
Copy link
Collaborator

@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.

It was a joy to review it, thanks @petarvujovic98 ! Now we are close to borsh serialization!

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.

BigInt serialization support
3 participants