near / near-api-js

JavaScript library to interact with NEAR Protocol via RPC API
https://near.github.io/near-api-js
MIT License
392 stars 245 forks source link

Consolidate and update Borsh for TS/JS #252

Closed MaksymZavershynskyi closed 3 years ago

MaksymZavershynskyi commented 4 years ago

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, https://github.com/nearprotocol/borsh/pull/57 and https://github.com/nearprotocol/borsh/pull/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.

kcole16 commented 4 years ago

@nearmax Is this the correct list of TODOs?

vgrichina commented 4 years ago

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

MaksymZavershynskyi commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago
  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 commented 4 years ago

@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 commented 4 years ago

Requires: https://github.com/near/borsh/issues/71

volovyks commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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