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(gnodev): txs manipulation ability #2286

Merged
merged 40 commits into from
Jul 8, 2024

Conversation

gfanton
Copy link
Member

@gfanton gfanton commented Jun 5, 2024

This PR adds the ability to manipulate transactions:

  • Users can cancel and redo a transaction from the terminal UI using the P and N commands.
  • Users can save the current state from the terminal UI using Ctrl+S and restore it later with Ctrl+R (reset command).
  • Users can export the current state in a genesis doc format from the terminal UI using the E command.
  • Users can load and extract transactions from any genesis file using the -genesis-file flag.

TODO:

  • Add documentation.
  • Add unit tests.
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.

@gfanton gfanton self-assigned this Jun 5, 2024
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 48.42105% with 98 lines in your changes missing coverage. Please review.

Project coverage is 54.90%. Comparing base (62fc9b4) to head (270e391).

Files Patch % Lines
contribs/gnodev/cmd/gnodev/main.go 0.00% 49 Missing ⚠️
contribs/gnodev/cmd/gnodev/setup_node.go 0.00% 24 Missing ⚠️
contribs/gnodev/pkg/dev/node_state.go 76.81% 9 Missing and 7 partials ⚠️
contribs/gnodev/pkg/dev/node.go 84.44% 4 Missing and 3 partials ⚠️
contribs/gnodev/pkg/rawterm/keypress.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2286      +/-   ##
==========================================
- Coverage   54.92%   54.90%   -0.02%     
==========================================
  Files         594      595       +1     
  Lines       79297    79447     +150     
==========================================
+ Hits        43550    43618      +68     
- Misses      32457    32533      +76     
- Partials     3290     3296       +6     
Flag Coverage Δ
contribs/gnodev 25.65% <48.42%> (+2.11%) ⬆️
contribs/gnofaucet 15.31% <ø> (+0.85%) ⬆️
contribs/gnokeykc 0.00% <ø> (ø)
contribs/gnomd 0.00% <ø> (ø)

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.

@gfanton gfanton requested a review from moul as a code owner June 5, 2024 18:42
Copy link
Contributor

@deelawn deelawn left a comment

Choose a reason for hiding this comment

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

Is there any way to just load a tx each time it starts without having to first export the entire state? Also, it doesn't seem consistent to have a separate flag for balances but not for transactions when they are both part of the genesis file. Maybe they could be all combined into the genesis file and let the user choose the file contents -- one of three options:

  • it is an actual genesis data struct
  • it is only balances
  • it is only txs

Though that could be confusing. I think it is fair to say that someone would want to only start the node with a single tx without having to first export the entire state -- much the same how someone would want to start the node with a single balance without having to first export the entire state. So maybe they should be three separate flags where the genesis file always gets loaded first and any additional balances or txs are appended.

@gfanton
Copy link
Member Author

gfanton commented Jun 6, 2024

@deelawn Good point. I will merge your PR (#2281) and then rebase this one on it. I will then update this PR so genesis-file also loads genesis balance, and let it be overridable by txs-file and/or balances-file.

@gfanton
Copy link
Member Author

gfanton commented Jun 7, 2024

Thinking more about this, I believe we should not authorize balance-file and/or txs-file along with genesis-file, as it would make things more complex and is likely not very useful. If we find a use case for this in the future, we can revisit and rework this part.

Signed-off-by: gfanton <[email protected]>
Copy link
Contributor

@deelawn deelawn left a comment

Choose a reason for hiding this comment

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

Nice job. I left a few nitpick comments and questions. The feature to apply and undo transactions is pretty cool 😎

contribs/gnodev/cmd/gnodev/main.go Outdated Show resolved Hide resolved
contribs/gnodev/cmd/gnodev/setup_node.go Outdated Show resolved Hide resolved
contribs/gnodev/pkg/dev/node.go Outdated Show resolved Hide resolved
contribs/gnodev/pkg/dev/node.go Outdated Show resolved Hide resolved
contribs/gnodev/pkg/dev/node.go Outdated Show resolved Hide resolved
@zivkovicmilos zivkovicmilos requested review from ajnavarro and removed request for leohhhn June 13, 2024 21:03
@Kouteki
Copy link
Contributor

Kouteki commented Jun 20, 2024

Waiting on @leohhhn to update the documentation

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.

Amazing feature, salute 🫡

Wen state viewer in gnodev

contribs/gnodev/cmd/gnodev/main.go Outdated Show resolved Hide resolved
contribs/gnodev/cmd/gnodev/main.go Outdated Show resolved Hide resolved
contribs/gnodev/cmd/gnodev/main.go Show resolved Hide resolved
contribs/gnodev/pkg/dev/node.go Outdated Show resolved Hide resolved
contribs/gnodev/pkg/dev/node.go Outdated Show resolved Hide resolved
contribs/gnodev/pkg/dev/node_state_test.go Outdated Show resolved Hide resolved
contribs/gnodev/pkg/dev/node_state_test.go Show resolved Hide resolved
contribs/gnodev/pkg/dev/node_state_test.go Outdated Show resolved Hide resolved
contribs/gnodev/pkg/dev/node_state_test.go Outdated Show resolved Hide resolved
contribs/gnodev/pkg/dev/node_state_test.go Show resolved Hide resolved
@zivkovicmilos
Copy link
Member

@gfanton
Just a nitpick, not all of us are fluent in Amino binary :)

Screenshot 2024-06-21 at 14 52 58

@gfanton
Copy link
Member Author

gfanton commented Jun 24, 2024

@zivkovicmilos I will updated this in a future PR. At the moment, it doesn't take into account the type of the event and just logs it with a %v. Improving gnodev log is definitely on my to-do list :)

Copy link
Contributor

@leohhhn leohhhn left a comment

Choose a reason for hiding this comment

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

Played around with it a bit, super cool. Docs look good 💯

@gfanton gfanton merged commit c5a79ab into gnolang:master Jul 8, 2024
79 checks passed
@gfanton gfanton deleted the feat/gnodev-handle-tx branch July 8, 2024 13:27
gfanton added a commit to gfanton/gno that referenced this pull request Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants