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-issues-242: borsh support #361

Merged
merged 6 commits into from
Aug 15, 2023
Merged

feat-issues-242: borsh support #361

merged 6 commits into from
Aug 15, 2023

Conversation

fospring
Copy link
Contributor

@fospring fospring commented Aug 2, 2023

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • Commit messages follow the conventional commits spec
  • If this is a code change: I have written unit tests.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Test Plan

install deps:

pnpm install
cd packages/near-sdk-js
pnpm install
pnpm run build
cd ../../examples
pnpm install

build and run test code:

pnpm run build:status-message-borsh
pnpm run test:status-message-borsh

outputs:

pnpm run test:status-message-borsh 

> examples@1.0.0 test:status-message-borsh /Users/qiuyongchun/workspace/near/near-sdk-js/examples
> ava __tests__/test-status-message-borsh.ava.js


Contract logs from dev-17943.test.near.get_status({"account_id":"test.near"}) view call: [ 'get_status for account_id test.near' ]
  ✔ Root gets null status (670ms)
Contract logs from dev-17943.test.near.set_status({"message":"hello"}) call: [ 'bob.test.near set_status with message hello' ]
Contract logs from dev-17943.test.near.set_status({"message":"hello"}) call: [ 'ali.test.near set_status with message hello' ]
Contract logs from dev-17943.test.near.get_status({"account_id":"ali.test.near"}) view call: [ 'get_status for account_id ali.test.near' ]
  ✔ Ali sets then gets status (4.1s)
Contract logs from dev-17943.test.near.set_status({"message":"world"}) call: [ 'carl.test.near set_status with message world' ]
Contract logs from dev-17943.test.near.get_status({"account_id":"bob.test.near"}) view call: [ 'get_status for account_id bob.test.near' ]
Contract logs from dev-17943.test.near.get_status({"account_id":"carl.test.near"}) view call: [ 'get_status for account_id carl.test.near' ]
  ✔ Bob and Carl have different statuses (7.3s)
  ─

  3 tests passed

Related issues/PRs

#242

return borshSerializeStatusMessage(value);
},
deserializer(value) {
return borshDeserializeStatusMessage(value);
Copy link
Member

Choose a reason for hiding this comment

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

Nice! Not related of this PR, but to ensure @NearBindgen(... serializer) API does work, can you try if intentionally throw error here, the test will fail due to the error?

Copy link
Contributor Author

@fospring fospring Aug 3, 2023

Choose a reason for hiding this comment

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

add throw Error test after new commits, test steps,
serialize throw error as expected:

pnpm run build:status-message-serialize-err
pnpm run test:status-message-serialize-err

outputs:

pnpm run test:status-message-serialize-err

> examples@1.0.0 test:status-message-serialize-err /Users/qiuyongchun/workspace/near/near-sdk-js/examples
> ava __tests__/test-status-message-serialize-err.ava.js


Contract logs from dev-13382.test.near.get_status({"account_id":"test.near"}) view call: [ 'get_status for account_id test.near' ]
  ✔ Root gets null status (683ms)
  ✔ Ali sets status (2.5s)
pnpm run build:status-message-deserialize-err
pnpm run test:status-message-deserialize-err

deserialize error outputs:

pnpm run test:status-message-deserialize-err

> examples@1.0.0 test:status-message-deserialize-err /Users/qiuyongchun/workspace/near/near-sdk-js/examples
> ava __tests__/test-status-message-deserialize-err.ava.js


Contract logs from dev-15825.test.near.get_status({"account_id":"test.near"}) view call: [ 'get_status for account_id test.near' ]
  ✔ Root gets null status (677ms)
Contract logs from dev-15825.test.near.set_status({"message":"hello"}) call: [ 'ali.test.near set_status with message hello' ]
  ✔ Ali sets then gets status (5s)
  ─

  2 tests passed

Copy link
Member

Choose a reason for hiding this comment

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

Perfect!

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.

Looks great! Let's just wait borsh make a new release and bump the version. This can be considering finished.

@ailisp
Copy link
Member

ailisp commented Aug 7, 2023

ci failed possibly due to this issue:  ERR_PNPM_LOCKFILE_CONFIG_MISMATCH. You can try the comment way or update ci configuration to use latest pnpm v8, and locally re-create lockfile

@gagdiez
Copy link
Collaborator

gagdiez commented Aug 11, 2023

@ailisp @fospring are we planning on setting borsh as the default (de)serializer? I noticed this PR only "showcases" that it is usable.

is there any plan to automatically generate the schema for the user?

@ailisp
Copy link
Member

ailisp commented Aug 14, 2023

@ailisp @fospring are we planning on setting borsh as the default (de)serializer? I noticed this PR only "showcases" that it is usable.

is there any plan to automatically generate the schema for the user?

for 1.0 this won't change this is a breaking change. For 2.0 we'll based on community input. At best both should be supported and unused serializer should be tree shaked

@gagdiez
Copy link
Collaborator

gagdiez commented Aug 14, 2023

@ailisp ok makes sense, this PR looks good to me.

@ailisp ailisp merged commit a0b8c44 into near:develop Aug 15, 2023
@ailisp ailisp mentioned this pull request Sep 12, 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.

4 participants