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

BREAKING CHANGE: add MakeCallTx, SignTx and BroadcastTxCommit in GnoNativeApi #180

Merged
merged 4 commits into from
Sep 24, 2024

Conversation

D4ryl00
Copy link
Contributor

@D4ryl00 D4ryl00 commented Sep 23, 2024

This PR adds MakeCallTx, SignTx and BroadcastTxCommit in GnoNativeApi. So the front can directly calls these methods now.
This PR also cleans the GnoNativeApi files, mostly by removing unnecessary await keywords.

BREAKING CHANGE: the gasWanted parameter (used in several gRPC message) is now a bigint instead of a number to keep the precision in the calling stack.

@D4ryl00 D4ryl00 requested a review from jefft0 September 23, 2024 12:21
@D4ryl00 D4ryl00 self-assigned this Sep 23, 2024
fnc: string,
args: string[],
gasFee: string,
gasWanted: bigint,
Copy link
Contributor

Choose a reason for hiding this comment

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

These parameters should be the same as call. In call, gasWanted is a number and is converted to bigint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I asked me the question. The best would be to do a changing break and also convert gasWanted as a bigint in the call function. Otherwise, we will limit the front end to put big values.
What do you think?

Copy link
Contributor

@jefft0 jefft0 Sep 23, 2024

Choose a reason for hiding this comment

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

In the Go code, these are normal integers. I don't know why the buf generator makes them bigint. (Maybe to make sure it can hold 64 bits.) I think we should as Iuri what he thinks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are int64 which is the equivalent of bigint in typescript.
https://github.com/gnolang/gnonative/blob/main/api/gnonativetypes/gnonativetypes.go#L251

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to keep as bigint because converting them to number could...

Be careful coercing values back and forth, however, as the precision of a BigInt value may be lost when it is coerced to a Number value

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt

@D4ryl00
Copy link
Contributor Author

D4ryl00 commented Sep 23, 2024

I added @iuricmp also to review the typescript part.

Copy link
Contributor

@jefft0 jefft0 left a comment

Choose a reason for hiding this comment

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

Tested by doing make npm.pack and installing in dSocial. Tested with the following code.

      const tx = await gnonative.makeCallTx("gno.land/r/berty/social", "AddReaction", args, gasFee, BigInt(gasWanted));
      const signedTx = await gnonative.signTx(tx.txJson, address);
      for await (const response of await gnonative.broadcastTxCommit(signedTx.signedTxJson)) {
        const result = JSON.parse(JSON.stringify(response)).result;
      }

Approved pending resolving the question of bigint vs. number.

fnc: string,
args: string[],
gasFee: string,
gasWanted: bigint,
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to keep as bigint because converting them to number could...

Be careful coercing values back and forth, however, as the precision of a BigInt value may be lost when it is coerced to a Number value

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt

@D4ryl00 D4ryl00 changed the title feat: add MakeCallTx, SignTx and BroadcastTxCommit in GnoNativeApi BREAKING CHANGE: add MakeCallTx, SignTx and BroadcastTxCommit in GnoNativeApi Sep 24, 2024
@D4ryl00 D4ryl00 merged commit f9b1aea into gnolang:main Sep 24, 2024
@D4ryl00 D4ryl00 deleted the feat/add-signTX-in-front branch September 24, 2024 08:29
Copy link

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants