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

chore: In gnoclient, separate out SignTx and BroadcastTxCommit #2641

Conversation

jefft0
Copy link
Contributor

@jefft0 jefft0 commented Jul 29, 2024

We can use gnokey maketx call, gnokey sign and gnokey broadcast to make a transaction and sign it separately before broadcast. We want to support the same thing in the gnoclient API. (And we want to expose this API in Gno Native Kit.)

  • Split out new API function NewCallTx from Call
  • Split out new API function NewRunTx from Run
  • Split out new API function NewSendTx from Send
  • Split out new API function NewAddPackageTx from AddPackage
  • Split out new API functions SignTx and BroadcastTxCommit from signAndBroadcastTxCommit
  • In client_test.go and integration_test.go, add code to also test signing separately
Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description

@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jul 29, 2024
Copy link

codecov bot commented Jul 29, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 6 lines in your changes missing coverage. Please review.

Project coverage is 60.24%. Comparing base (b7dbed9) to head (b7631b1).
Report is 1 commits behind head on master.

Files Patch % Lines
gno.land/pkg/gnoclient/client_txs.go 84.61% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2641      +/-   ##
==========================================
+ Coverage   60.19%   60.24%   +0.04%     
==========================================
  Files         562      562              
  Lines       75007    75038      +31     
==========================================
+ Hits        45154    45210      +56     
+ Misses      26476    26457      -19     
+ Partials     3377     3371       -6     
Flag Coverage Δ
contribs/gnodev 61.40% <ø> (+0.81%) ⬆️
contribs/gnofaucet 14.46% <ø> (ø)
gno.land 64.57% <84.61%> (+0.10%) ⬆️
gnovm 64.33% <ø> (ø)
misc/genstd 80.54% <ø> (ø)
misc/logos 19.88% <ø> (ø)
tm2 62.05% <ø> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…d BroadcastTxCommit

Also add MakeCallTx, MakeRunTx, MakeSendTx and MakeAddPackageTx to support signing separately.

Signed-off-by: Jeff Thompson <jeff@thefirst.org>
@jefft0 jefft0 force-pushed the chore/gnoclient-add-SignTx-BroadcastTxCommit-MakeCallTx-etc branch from 795277a to 10550df Compare July 31, 2024 10:34
jefft0 added a commit to gnolang/gnonative that referenced this pull request Aug 2, 2024
…use temporary replace for gno PR 2641 (#161)

PR gnolang/gno#2641 updates the `gnoclient` API
to add `MakeCallTx`, etc. and `SignTx` and `BroadcastTxCommit`. We want
to support these in the gnonative API.
* In go.mod, add a replace for github.com/gnolang/gno to the branch of
PR gnolang/gno#2641
* In gnonativetypes.go, add field `CallerAddress` to `CallRequest`,
`SendRequest` and `RunRequest`. Add `MakeTxResponse` and
request/response for `SignTx` and `BroadcastTxCommitResponse`.
* In rpc.proto, add `MakeCallTx`, etc., `SignTx` and
`BroadcastTxCommit`.
* Run `make regenerate` .
* In api.go, split out helper function `convertCallRequest` from `Call`.
Do the same for `convertSendRequest` and `convertRunRequest`. Add
`MakeCallTx` which also uses `convertCallRequest`. Do the same for
`MakeSendTx` and `MakeRunTx`. Add `SignTx` and `BroadcastTxCommit`.
* Note that `MakeCallTx` converts the tx structure to JSON using
`amino.MarshalJSON(tx)` . This is the same thing as done by the
command-line `gnokey maketx call` .

Signed-off-by: Jeff Thompson <jeff@thefirst.org>
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

I think this is a huge step for the gnoclient, one we should've made for v1 of it 💯

I'm sorry for making you fix legacy code here, but I think we can fix this signing insanity once and for all. The changes look solid 🚀

gno.land/pkg/gnoclient/client_txs.go Outdated Show resolved Hide resolved
gno.land/pkg/gnoclient/client_txs.go Show resolved Hide resolved
gno.land/pkg/gnoclient/client_txs.go Outdated Show resolved Hide resolved
gno.land/pkg/gnoclient/client_txs.go Outdated Show resolved Hide resolved
gno.land/pkg/gnoclient/client_txs.go Outdated Show resolved Hide resolved
gno.land/pkg/gnoclient/integration_test.go Show resolved Hide resolved
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Thank you for the cleanup 🙏

@zivkovicmilos
Copy link
Member

Pinging @gfanton for a second review 🙏

Copy link
Member

@gfanton gfanton 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!
Would it make sense to expose SignAndBroadcastTx?

@jefft0
Copy link
Contributor Author

jefft0 commented Aug 23, 2024

Looks good! Would it make sense to expose SignAndBroadcastTx?

Thanks for the review. No, I don't think we need to expose SignAndBroadcastTx. The use case for separating out SignTx is for airgap signing (or signing with a different app like Gnokey Mobile). In this use case, sign and broadcast are done on different devices or apps.

@zivkovicmilos zivkovicmilos merged commit 651f5aa into gnolang:master Aug 26, 2024
119 checks passed
jefft0 added a commit to gnolang/gnonative that referenced this pull request Aug 29, 2024
Gno PR gnolang/gno#2641 has been merged, so we
don't need the go.mod replace which was introduced in PR
#161 .

* In go.mod, remove the replace and use the latest gnolang/gno.
* The final gnoclient API which was merged in PR
gnolang/gno#2641 is different. It uses
`vm.MsgCall`, etc. instead of `gnoclient.MsgCall`, and uses `NewCallTx`,
etc. instead of `MakeCallTx`. Therefore, this PR updates api.go to use
the new API.
* Change the helper functions `convertCallRequest`, etc. to maybe return
an error if there is no active account, or if the `send` amount like
"1000ugnot" can't be parsed.

Signed-off-by: Jeff Thompson <jeff@thefirst.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants