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(gnokey): Print out the transaction hash when maketx executes successfully #2309

Merged
merged 59 commits into from
Jul 22, 2024

Conversation

linhpn99
Copy link
Contributor

@linhpn99 linhpn99 commented Jun 8, 2024

Relate to #2303

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.

@linhpn99 linhpn99 requested review from moul, jaekwon and a team as code owners June 8, 2024 03:27
@github-actions github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Jun 8, 2024
@linhpn99 linhpn99 changed the title Print out the transaction hash when maketx executes successfully feat(gnokey): Print out the transaction hash when maketx executes successfully Jun 8, 2024
Copy link

codecov bot commented Jun 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 55.00%. Comparing base (1180def) to head (79d5f22).

Files Patch % Lines
tm2/pkg/crypto/keys/client/broadcast.go 0.00% 2 Missing ⚠️
tm2/pkg/crypto/keys/client/maketx.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2309      +/-   ##
==========================================
- Coverage   55.01%   55.00%   -0.01%     
==========================================
  Files         595      595              
  Lines       79727    79731       +4     
==========================================
  Hits        43858    43858              
- Misses      32550    32554       +4     
  Partials     3319     3319              
Flag Coverage Δ
contribs/gnodev 26.00% <ø> (ø)
contribs/gnofaucet 14.46% <ø> (-0.86%) ⬇️
contribs/gnokeykc 0.00% <ø> (ø)
contribs/gnomd 0.00% <ø> (ø)
gno.land 64.24% <ø> (ø)
tm2 54.45% <0.00%> (+0.01%) ⬆️

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.

@linhpn99 linhpn99 requested review from gfanton, zivkovicmilos and a team as code owners June 8, 2024 03:47
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 addition 🙏

We just need to change the display encoding, and we should be good to go 🚀

tm2/pkg/crypto/keys/client/broadcast.go Outdated Show resolved Hide resolved
tm2/pkg/crypto/keys/client/maketx.go Outdated Show resolved Hide resolved
@zivkovicmilos
Copy link
Member

@linhpn99 can you please merge in the master branch? 🙏

@moul
Copy link
Member

moul commented Jun 12, 2024

Can we have at least one unit test not checking that "a string is printed", but actually checking that the hash looks correct?

@linhpn99
Copy link
Contributor Author

Can we have at least one unit test not checking that "a string is printed", but actually checking that the hash looks correct?

for sure, i'm working on it

@linhpn99
Copy link
Contributor Author

linhpn99 commented Jun 13, 2024

Can we have at least one unit test not checking that "a string is printed", but actually checking that the hash looks correct?

However, with the current code structure, it is not possible to write unit tests due to the need to mock certain components (like rpcClient). I think it would be better to create a separate PR to address this issue, which would include the necessary mocking and test cases for the missing functions. wdyt @moul @zivkovicmilos ?

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.

Looks good now 💯

I've left a small comment where we can additionally print the hash, otherwise we're good to go 🚀

tm2/pkg/crypto/keys/client/broadcast.go Show resolved Hide resolved
tm2/pkg/crypto/keys/client/maketx.go Show resolved Hide resolved
@zivkovicmilos zivkovicmilos merged commit 1fbd373 into gnolang:master Jul 22, 2024
81 of 83 checks passed
gfanton pushed a commit to gfanton/gno that referenced this pull request Jul 23, 2024
…cessfully (gnolang#2309)

Relate to gnolang#2303

<!-- please provide a detailed description of the changes made in this
pull request. -->

<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
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[gnokey] Display transaction hash to user after a maketx call
3 participants