Skip to content
This repository has been archived by the owner on Oct 20, 2020. It is now read-only.

feat: add mint_with_json #8

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

feat: add mint_with_json #8

wants to merge 1 commit into from

Conversation

chadoh
Copy link

@chadoh chadoh commented Sep 11, 2020

This uses JSON serialization rather than borsh, which is significantly easier to use from a browser with near-api-js, including providing much better error messaging

This uses JSON serialization rather than borsh, which is significantly
easier to use from a browser with near-api-js, including providing much
better error messaging
@chadoh chadoh requested a review from ailisp September 11, 2020 20:12
@chadoh chadoh self-assigned this Sep 11, 2020
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 good to me. @nearmax was there a reason to use borsh or to use json serailize, we seem to have a mixed of them in rainbow-bridge-rs contracts

@ailisp
Copy link
Member

ailisp commented Sep 11, 2020

Also, as all changes before, please don't forget to increase version in package.json :D

@chadoh
Copy link
Author

chadoh commented Sep 12, 2020

Note: I would rather change mint outright, but adding a new method was an easier way to get erc20-to-nep21 working without breaking existing libraries

If someone wants to modify the main mint function, or replace the token-specific connectors with the generic version in https://github.com/near/rainbow-token-connector (assuming it uses JSON on frontend-facing functions), that would be 10x better

Copy link

@MaksymZavershynskyi MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

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

JSON serialization of data-heavy objects is very expensive, it might even dominate the cost of the contract for proofs constructed from real headers from Ropsten of Eth Mainnet. We prefer using borsh for this very reason when we are passing headers, proofs and other data-heavy arguments. Alsol the way JSON serializes byte arrays is an array of integers, which is very expensive to parse. CC @Kouprin I don't think it is a good idea to pass it with JSON. Overall, for changes that are suspected to change the gas cost it is recommended to try the contract on the real NEAR node with real data, e.g. proofs from headers on Ropsten or Mainnet.

@Kouprin
Copy link
Member

Kouprin commented Sep 12, 2020

Am I right is affects user experience significantly? More precisely:

  1. It's easy to start developing with JSON.
  2. For business purposes, it's important to switch to Borsh.

I'd suggest to discuss having both Borsh and JSON ser/deser. Three cons:

  1. Easy onboarding because of JSON.
  2. We have very limited number of methods that must be Borshified. It will be not too hard to support them.
  3. Don't lose possibility to switch to Borsh and save gas.

@chadoh chadoh marked this pull request as draft September 18, 2020 00:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants