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

CLI spend command makes exact change #593

Merged
merged 1 commit into from
Sep 20, 2022

Conversation

jkitman
Copy link
Contributor

@jkitman jkitman commented Sep 19, 2022

The receive_coins function is the way we should transfer ecash in production because it's the safer/faster option.

However, for demonstrating ecash as a bearer note, this changes the spend_ecash function so that it will reissue exact change for spending.

Fixes #592

@@ -106,6 +106,11 @@ where
&self,
transaction: Transaction,
) -> Result<(), TransactionSubmissionError> {
// we already processed the transaction before the request was received
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discovered this bug when an epoch is processed before the submit_transaction occurs.

@jkitman jkitman force-pushed the spend-command-makes-exact-change branch 2 times, most recently from 98a9d4d to a8746cd Compare September 19, 2022 17:47
@jkitman jkitman marked this pull request as ready for review September 19, 2022 18:41
@justinmoon
Copy link
Contributor

Besides my 1 comment this looks good. And my comment is more of an optimization.

@jkitman jkitman force-pushed the spend-command-makes-exact-change branch from a8746cd to f47d493 Compare September 20, 2022 11:39
@codecov-commenter
Copy link

Codecov Report

Base: 72.38% // Head: 72.43% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (f47d493) compared to base (520e118).
Patch coverage: 88.88% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #593      +/-   ##
==========================================
+ Coverage   72.38%   72.43%   +0.04%     
==========================================
  Files          85       85              
  Lines       11516    11543      +27     
==========================================
+ Hits         8336     8361      +25     
- Misses       3180     3182       +2     
Impacted Files Coverage Δ
client/cli/src/main.rs 0.30% <0.00%> (ø)
client/clientd/src/main.rs 0.83% <0.00%> (-0.01%) ⬇️
fedimint/src/consensus/mod.rs 92.76% <80.00%> (-0.15%) ⬇️
client/client-lib/src/lib.rs 86.45% <96.66%> (+0.28%) ⬆️
client/client-lib/src/mint/mod.rs 81.87% <100.00%> (+1.25%) ⬆️
client/client-lib/src/transaction.rs 99.20% <100.00%> (ø)
fedimint/src/net/peers.rs 92.44% <0.00%> (-0.53%) ⬇️
fedimint/src/net/connect.rs 94.94% <0.00%> (-0.26%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jkitman jkitman requested a review from justinmoon September 20, 2022 13:30
@justinmoon
Copy link
Contributor

LGTM. @elsirion would you take a look, too?

Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

LGTM. Would be nice if the RNG change and the tx submission fix were separate commits for ease of review.

@elsirion elsirion merged commit 884206f into fedimint:master Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI spend command should make exact change
4 participants