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

feat(gnoclient): add MultiCall #1565

Merged
merged 45 commits into from
Feb 1, 2024

Conversation

leohhhn
Copy link
Contributor

@leohhhn leohhhn commented Jan 20, 2024

Description

This PR introduces a the MultiCall feature to the Gnoclient. It allows for packing multiple vm.MsgCall messages into a single transaction, then signing & publishing it.

The PR utilizes the same API, just allows for multiple MsgCall messages in the CallCfg struct, and parses accordingly.

cc @zivkovicmilos @moul @jefft0

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
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

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

codecov bot commented Jan 20, 2024

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (cfd5d33) 56.13% compared to head (60b5963) 47.25%.
Report is 1 commits behind head on master.

Files Patch % Lines
gno.land/pkg/gnoclient/util.go 43.75% 6 Missing and 3 partials ⚠️
gno.land/pkg/gnoclient/client_txs.go 73.33% 6 Missing and 2 partials ⚠️
gno.land/pkg/gnoclient/client.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1565      +/-   ##
==========================================
- Coverage   56.13%   47.25%   -8.88%     
==========================================
  Files         438      377      -61     
  Lines       66151    61322    -4829     
==========================================
- Hits        37134    28980    -8154     
- Misses      26126    29953    +3827     
+ Partials     2891     2389     -502     
Flag Coverage Δ
go-1.21.x ∅ <ø> (∅)
misc ∅ <ø> (∅)
misc-_test.genstd ∅ <ø> (∅)

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.

@leohhhn leohhhn marked this pull request as ready for review January 22, 2024 10:07
@leohhhn leohhhn requested a review from moul as a code owner January 22, 2024 10:07
@zivkovicmilos zivkovicmilos self-requested a review January 22, 2024 12:26
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.

Please see the comments I've left 🙏

I don't think we need another Call method just to support multiple message types, as the functionality is identical, but that we should see how to utilize existing client functionality. The API of any client should be very specific and lean, not wide to accommodate different use-cases. Keep the implementation deep, but the API shallow.

Also, please add unit tests for this functionality 🙏

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
@leohhhn
Copy link
Contributor Author

leohhhn commented Jan 23, 2024

@zivkovicmilos added some tests: fb2416d

@leohhhn
Copy link
Contributor Author

leohhhn commented Jan 24, 2024

Putting this on hold until we figure out what the execution semantics and implications are when calling multiple MsgCall Messages in a single transaction. Primary concerns:

  • If a single MsgCall reverts, will the whole transaction revert?
  • What is the execution order of the MsgCalls? If two MsgCalls are working with the same memory, which will be executed first? Can the second one pick up the change done by the first one?
  • etc.

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.

Progress, but still not fully there. We are missing critical unit tests for the functionality.

Please see my comments 🙏

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/errors.go Outdated Show resolved Hide resolved
gno.land/pkg/gnoclient/client_test.go Show resolved Hide resolved
gno.land/pkg/gnoclient/client_test.go Outdated Show resolved Hide resolved
gno.land/pkg/gnoclient/client_test.go Outdated Show resolved Hide resolved
gno.land/pkg/gnoclient/client_test.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
@zivkovicmilos
Copy link
Member

Putting this on hold until we figure out what the execution semantics and implications are when calling multiple MsgCall Messages in a single transaction. Primary concerns:

  • If a single MsgCall reverts, will the whole transaction revert?
  • What is the execution order of the MsgCalls? If two MsgCalls are working with the same memory, which will be executed first? Can the second one pick up the change done by the first one?
  • etc.
  1. Yes, the entire transaction should revert
  2. As far as I'm aware, the execution order of messages within a transaction is as they are specified. A former message is working off of the state from the last message during the execution cycle.

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 resolving the comments 🙏

I've left final thoughts, which are easy fixes.
Otherwise we should be good to go 🚀

I appreciate you taking the time to resolve everything and learn in the process. Great work 👏

gno.land/pkg/gnoclient/client_test.go Show resolved Hide resolved
gno.land/pkg/gnoclient/client_test.go Outdated Show resolved Hide resolved
gno.land/pkg/gnoclient/client_test.go Outdated Show resolved Hide resolved
gno.land/pkg/gnoclient/mock.go Outdated Show resolved Hide resolved
gno.land/pkg/gnoclient/mock.go Outdated Show resolved Hide resolved
gno.land/pkg/gnoclient/mock.go Outdated Show resolved Hide resolved
@thehowl thehowl merged commit d8a6dec into gnolang:master Feb 1, 2024
65 of 67 checks passed
leohhhn added a commit to leohhhn/gno that referenced this pull request Feb 2, 2024
## Description

This PR introduces a the MultiCall feature to the Gnoclient. It allows
for packing multiple vm.MsgCall messages into a single transaction, then
signing & publishing it.

The PR utilizes the same API, just allows for multiple MsgCall messages
in the CallCfg struct, and parses accordingly.

cc @zivkovicmilos @moul @jefft0

<details><summary>Contributors' checklist...</summary>

- [ ] 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
- [ ] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
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
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants