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

Make CLI spend e-cash more configurable #3902

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

elsirion
Copy link
Contributor

@elsirion elsirion commented Dec 11, 2023

Fixes #3834

Copy link

codecov bot commented Dec 11, 2023

Codecov Report

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

Comparison is base (e882197) 57.13% compared to head (868f675) 57.09%.

Files Patch % Lines
fedimint-cli/src/client.rs 0.00% 25 Missing ⚠️
modules/fedimint-mint-client/src/lib.rs 0.00% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3902      +/-   ##
==========================================
- Coverage   57.13%   57.09%   -0.04%     
==========================================
  Files         193      193              
  Lines       42861    42897      +36     
==========================================
+ Hits        24487    24492       +5     
- Misses      18374    18405      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@elsirion elsirion marked this pull request as ready for review December 11, 2023 18:13
@elsirion elsirion requested a review from a team as a code owner December 11, 2023 18:13
allow_overpay: bool,
/// After how many seconds we will try to reclaim the e-cash if it
/// hasn't been redeemed by the recipient
#[clap(long, default_value_t = 3600)]
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Is this automatic behavior? If so, isn't this a bit low for a default for something that might be passed in a DM or even piece of paper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, what would you propose? A week?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable.

BTW. Maybe we should also eprintln! the deadline to warn/educate the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could that be a warn level log? These are still shown and should be printed to stderr anyway (if not we should fix that).

Copy link
Contributor

Choose a reason for hiding this comment

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

The way I think about it is: logging is for reporting what the program is doing, while (e)println for the actual output for the user. Not feeling too strongly about it one way or another.

Copy link
Contributor Author

@elsirion elsirion Dec 11, 2023

Choose a reason for hiding this comment

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

We need a fourth stream, stdlog! 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

In the CLI what would trigger this timeout path to be taken? Any other CLI command run after this timeout has passed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, basically anything that runs the SM executor for long enough. What I like to do to trigger this manually is create a LN invoice I don't intend to pay and then await it. We might want a more general purpose "run executor" command.

dpc
dpc previously approved these changes Dec 11, 2023
douglaz
douglaz previously approved these changes Dec 12, 2023
Copy link
Contributor

@douglaz douglaz left a comment

Choose a reason for hiding this comment

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

Requires rebase, but LGTM

Previously the spend command would return the next-higher amount if the exact
 amount could not be represented with the available denominations.
@elsirion elsirion added this pull request to the merge queue Dec 12, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 12, 2023
@elsirion elsirion added this pull request to the merge queue Dec 12, 2023
Merged via the queue into fedimint:master with commit ce876ae Dec 12, 2023
20 checks passed
@elsirion elsirion deleted the 2023-12-spend branch December 12, 2023 14:07
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.

fedimint-cli spend should have a flag to give exact change or error if it can't
5 participants