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

Consolidate and update Borsh for TS/JS #252

Closed
MaksymZavershynskyi opened this issue Mar 10, 2020 · 11 comments
Closed

Consolidate and update Borsh for TS/JS #252

MaksymZavershynskyi opened this issue Mar 10, 2020 · 11 comments
Labels
enhancement New feature or request P1 Very important

Comments

@MaksymZavershynskyi
Copy link
Contributor

MaksymZavershynskyi commented Mar 10, 2020

Currently we have three different implementation of TS/JS that have slightly different functionality and bugs:

We should unify them into one implementation that lives in https://github.com/nearprotocol/borsh .

Also, near/borsh#57 and near/borsh#56 introduced a slightly modified Borsh schema format which now allows serializing any Rust types, even with generics, and it is not Rust-specific. We should make sure we follow this new schema format.

Note, as requested by @vgrichina Borsh Schema is in Borsh format rather than JSON.

@MaksymZavershynskyi MaksymZavershynskyi transferred this issue from near/create-near-app Mar 10, 2020
@MaksymZavershynskyi MaksymZavershynskyi added the P1 Very important label Mar 10, 2020
@kcole16
Copy link

kcole16 commented Mar 10, 2020

@nearmax Is this the correct list of TODOs?

@vgrichina
Copy link
Contributor

There are few important points that shouldn't be missed:

@MaksymZavershynskyi
Copy link
Contributor Author

@kcole16 I don't know how I missed your comment.

@nearmax Is this the correct list of TODOs?

Yes, correct.

@vgrichina

borsh-ts is most out of date and untested implementation, but it ultimately should be destination where new implementation lands

I agree.

improves one thing which is important to fix elsewhere – Borsh schema doesn't depend on JS constructors anymore

This is an important improvement, which will allow us in the future to directly convert JSON to Borsh using Borsh schema


@vgrichina Please also note that this is not blocking our Mainnet launch, so we have removed it from Mainnet release in Zenhub last week. If you have other items that are blocking the Mainnet launch then you might consider them more important.

@vgrichina
Copy link
Contributor

@nearmax re mainnnet priorities we likely need to do smth about RPCs. Looks like long polling on nearcore side works bad (e. g. tx-status for non existing hash). This will require changes both in nearcore and in nearlib, though should be pretty localized.

@MaksymZavershynskyi
Copy link
Contributor Author

@nearmax re mainnnet priorities we likely need to do smth about RPCs. Looks like long polling on nearcore side works bad (e. g. tx-status for non existing hash). This will require changes both in nearcore and in nearlib, though should be pretty localized.

Yes, but these changes look orthogonal to Borsh changes. CC @frol

@frol
Copy link
Collaborator

frol commented Mar 25, 2020

  1. This issue indeed looks orthogonal to the RPC.
  2. I agree that RPC and the whole node need benchmarks () and load testing. We have tracking issues for those and will work on those items in the near future.
  3. RPC polling is never a good idea, and we will need to implement some streaming APIs, but this can be done after the mainnet, I believe.

@MaksymZavershynskyi
Copy link
Contributor Author

@kcole16 , @vgrichina We need to bump priority on this. Flux has been asking examples on JS-side Borsh serialization, which we cannot do until we consolidate different implementations.

@MaksymZavershynskyi MaksymZavershynskyi changed the title Reconcile and update Borsh for TS/JS Consolidated and update Borsh for TS/JS Apr 30, 2020
@MaksymZavershynskyi MaksymZavershynskyi changed the title Consolidated and update Borsh for TS/JS Consolidate and update Borsh for TS/JS Apr 30, 2020
@MaksymZavershynskyi MaksymZavershynskyi added Epic Milestone or collection of like issues and removed Epic Milestone or collection of like issues labels Apr 30, 2020
@MaksymZavershynskyi
Copy link
Contributor Author

Requires: near/borsh#71

@volovyks
Copy link
Collaborator

volovyks commented Feb 24, 2021

@vgrichina @nearmax this issues is a bit outdated

should we consider Update borsh-ts and borsh-js to the new schema as a P1?

@vgrichina
Copy link
Contributor

@volovyk-s not sure it’s a P1 but it’s relatively important as would solve the problem with using multiple near-api-js instances in your app. Basically the idea is to avoid relying on constructor instances to identify schema.

@volovyks
Copy link
Collaborator

Closing this issue since it's not relevant to near-api-js anymore. borsh-js schema should be fixed in near/borsh-js#10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P1 Very important
Projects
None yet
Development

No branches or pull requests

6 participants