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

fix(gnofaucet): fix transactions by querying account number and sequence each time #1493

Merged

Conversation

jefft0
Copy link
Contributor

@jefft0 jefft0 commented Jan 2, 2024

Currently, gnofaucet queries the blockchain once for the account number and sequence number, and increments the sequence number each time that it sends coins. We are using gnofaucet with gnodev. When gnodev restarts the chain, it also resets the sequence number. This means that we also have to restart gnofaucet (to re-query the sequence number), which is inconvenient for the development workflow.

If you agree that it would be nice for gnofaucet to continue to work after gnodev does a reset, then this pull request moves the query for the account number and sequence number into the sendAmountTo function so that it queries each time it signs a transaction. Now gnofaucet continues to function even if the sequence number is unexpectedly changed (because the blockchain is reset or because some other transaction is signed with the same key).

Signed-off-by: Jeff Thompson <jeff@thefirst.org>
@jefft0 jefft0 requested a review from a team as a code owner January 2, 2024 11:55
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jan 2, 2024
Copy link

codecov bot commented Jan 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c3a4993) 55.85% compared to head (3e2026d) 56.11%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1493      +/-   ##
==========================================
+ Coverage   55.85%   56.11%   +0.26%     
==========================================
  Files         431      432       +1     
  Lines       65729    65971     +242     
==========================================
+ Hits        36713    37022     +309     
+ Misses      26140    26058      -82     
- Partials     2876     2891      +15     
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.

@jefft0
Copy link
Contributor Author

jefft0 commented Jan 11, 2024

We are now using the new faucet at https://github.com/gnolang/faucet which doesn't have this problem. So this PR to fix the old gnofaucet would be reviewed with low priority (or never). Should I close this PR now?

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.

Confirming this is broken in the deprecated faucet implementation (in this repo), it's something we've fixed in the new faucet

Thank you for the fix 🙏

@thehowl thehowl changed the title chore: In gnofaucet, query for the account number and sequence each time fix(gnofaucet): fix transactions by querying account number and sequence each time Jan 11, 2024
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Approving and merging. Even though it's a fix for a deprecated component, it still improves our codebase's situation, and this PR is better merged than closed :)

Thanks!

@thehowl thehowl merged commit bb63e77 into gnolang:master Jan 11, 2024
184 checks passed
gfanton pushed a commit to moul/gno that referenced this pull request Jan 18, 2024
…nce each time (gnolang#1493)

Currently, `gnofaucet` [queries the blockchain
once](https://github.com/gnolang/gno/blob/c3a4993335f8881989eb49e4efea6a3c1310061d/gno.land/cmd/gnofaucet/serve.go#L191-L209)
for the account number and sequence number, and [increments the sequence
number](https://github.com/gnolang/gno/blob/master/gno.land/cmd/gnofaucet/serve.go#L327)
each time that it sends coins. We are using `gnofaucet` with `gnodev`.
When `gnodev` restarts the chain, it also resets the sequence number.
This means that we also have to restart `gnofaucet` (to re-query the
sequence number), which is inconvenient for the development workflow.

If you agree that it would be nice for `gnofaucet` to continue to work
after `gnodev` does a reset, then this pull request moves the query for
the account number and sequence number into the `sendAmountTo` function
so that it queries each time it signs a transaction. Now `gnofaucet`
continues to function even if the sequence number is unexpectedly
changed (because the blockchain is reset or because some other
transaction is signed with the same key).

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
Status: No status
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants