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.

Sign up for GitHub

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

Jump to bottom

fix!: refactor bindgen and update node versions #581

Closed
wants to merge 13 commits into from
Closed

fix!: refactor bindgen and update node versions #581

wants to merge 13 commits into from

Conversation

willemneal
Copy link
Contributor

@willemneal willemneal commented Jun 9, 2021 •
edited

This PR updates the bindings generation for contracts to use the new and improved serialization framework serial-as. The steps of the transform are now broken up into their steps. It also opens up the use of borsh serialization.

Changes

Initialization check required

One big change that serial-as makes is to the semantics of class declarations. Assemblyscript like Typescript requires that fields that are not nullable (e.i. string | null) to be initialized. Previously the bindgen code got around this requirement but calling a defaultValue<T>() function which would get around this check and allow non-nullables to be set as zero.

For example,

class A {
 i: i32 = 0
}

class Foo {
  a: A;
}

let f = new Foo(); // This should fail to compile because there is no initializer for `a`.

// The bindgen code would change this to 

class Foo {
  a: A = defaultValue<A>(); // Does `changetype<A>(0)` <-- unsafe `null`
}

// Can still have empty initializer if nullable

class Foo {
  a: A | null;
}
// Now it is clear that the field might be null whereas before it was assumed not be.

This original transform was there for backwards compatibility to previous versions of AS which didn't have the initialization check.

However, this led to encouraging bad and unsafe practices and thus now all fields are required to be initialized.

Node versions

This PR also removes compatibility to node versions 12 and 13. The former does not support optional chaining x?.foo(), and the later is dropped by newer versions of jest.

@chadoh chadoh self-requested a review July 28, 2021 15:56
@willemneal willemneal changed the title chore: initial refactor fix!: refactor bindgen and update node versions Jul 28, 2021
chadoh
chadoh suggested changes Jul 28, 2021
View reviewed changes
Copy link
Contributor

@chadoh chadoh left a comment

Choose a reason for hiding this comment

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

See some notes below. I think there's at least some minor cleanup & issues to address prior to merging. I also don't see any doc updates, and some changes aren't mentioned in the PR description (which seems like a good place to "stage" release notes, and we'll definitely want this stuff mentioned in the release notes with the next major version).

.gitignore
@@ -28,3 +28,4 @@ build/
sim-ffi/sim-ffi/target/

sim-ffi/sim-ffi/index.node
sim-ffi
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could remove the previous two file patterns here

bindgen/assembly/index.ts
// " is required but missing"
// );
// return __defaultValue<T>();
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

should we remove the comments before merge?

bindgen/assembly/index.ts
// }
// if (isString<T>()) return "";
// return changetype<T>(0);
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

can rm comment?

bindgen/package.json
@@ -35,6 +35,10 @@
"assembly/index.ts"
],
"devDependencies": {
"near-sdk-core": "^3.2.1"
"near-sdk-core": "^3.2.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

reverting to an older version of sdk-core?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
"near-sdk-core": "^3.2.0"
"near-sdk-core": "^3.2.1"

bindgen/src/JSONBuilder.ts
// builder.visit(sources);
// return sources;
// }
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

can rm comments (whole file)?

bindgen/src/transformer.ts
import { JSONBindingsBuilder, isEntry } from "./JSONBuilder";
import { TypeChecker } from "./typeChecker";
import { Transform, Parser, Program, Module, SourceKind } from "visitor-as/as";
// import { JSONBindingsBuilder } from "./JSONBuilder";
Copy link
Contributor

Choose a reason for hiding this comment

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

can rm comment?

sdk-core/assembly/index.ts
@@ -9,5 +9,8 @@ export * from "./logging";
export * from "./math";
export * from "./promise";
export * from "./datetime";
import * as JSON from "@serial-as/json";
export { JSON };
export * from "@serial-as/borsh";
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the implications of this change? I didn't see it mentioned in any updated docs or in the PR description (do we use the PR description to create release notes? might be useful)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the future branch this isn't needed because we have a serializer.ts which exports a simplified serializer. So I think I'll add that to this PR. And add to PR description. I also think we should make a CHANGELOG.md to formalize this.

sdk/assembly/__tests__/bignum.spec.ts
@@ -1,18 +1,22 @@
import { u128 } from "..";

// TODO: findout why the commented lines below fail
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this TODO comment needs to be addressed prior to merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good point. bignum didn't fail in any of the other tests so I saw this as a future work to understand this edge case.

@willemneal willemneal closed this Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chadoh chadoh chadoh requested changes

@mehtaphysical mehtaphysical Awaiting requested review from mehtaphysical

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@willemneal @chadoh