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

NUT-04 Mint Quote Description #337

Merged
merged 14 commits into from
Sep 12, 2024

Conversation

lollerfirst
Copy link
Contributor

No description provided.

Copy link
Collaborator

@thesimplekid thesimplekid left a comment

Choose a reason for hiding this comment

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

Thanks left a few comments.

crates/cdk/src/nuts/nut04.rs Show resolved Hide resolved
crates/cdk-axum/src/router_handlers.rs Show resolved Hide resolved
crates/cdk/src/wallet/mod.rs Outdated Show resolved Hide resolved
crates/cdk/src/nuts/nut04.rs Outdated Show resolved Hide resolved
@@ -254,6 +256,7 @@ impl Default for Settings {
unit: CurrencyUnit::Sat,
min_amount: Some(Amount::from(1)),
max_amount: Some(Amount::from(1000000)),
quote_description: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could return here the following depending on whether the backend supports descriptions?

quote_description: ln.get_settings().invoice_description

Of course, we would need to get the value here, the code above is just a pseudo-code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We won't be able to do that here in default() so I think the default should be false and then here when setting MintMethofSettings for each backend in mintd we shouldn't use the default but actually set it based on what the backend supports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't the default be true as this is the case for the vast majority of backends?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think false since even if a backend can support it the mint operator may not want to and it should be explicitly enabled.

@@ -185,7 +185,7 @@ pub async fn mint_proofs(
let wallet_client = HttpClient::new();

let mint_quote = wallet_client
.post_mint_quote(mint_url.parse()?, 1.into(), CurrencyUnit::Sat)
.post_mint_quote(mint_url.parse()?, 1.into(), CurrencyUnit::Sat, None)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we maybe try to pass description here if it is supported by the mint?

@thesimplekid
Copy link
Collaborator

Sorry about the conflicts. If you rebase onto main it shouldn't be too bad all the ones with conflicts should just be deleted except crates/cdk-integration-tests/tests/mint.rs and I think that should just be as is on main

@lollerfirst lollerfirst force-pushed the mint-quote-description branch from 95c45eb to b975317 Compare September 10, 2024 18:52
Co-authored-by: thesimplekid <tsk@thesimplekid.com>
lollerfirst and others added 2 commits September 10, 2024 22:08
Co-authored-by: thesimplekid <tsk@thesimplekid.com>
@prusnak
Copy link
Contributor

prusnak commented Sep 11, 2024

these two changes need to be applied to fix failures in the CI:

diff --git a/bindings/cdk-js/src/wallet.rs b/bindings/cdk-js/src/wallet.rs
index 3c91f71..5b65556 100644
--- a/bindings/cdk-js/src/wallet.rs
+++ b/bindings/cdk-js/src/wallet.rs
@@ -89,7 +89,7 @@ impl JsWallet {
     pub async fn mint_quote(&mut self, amount: u64) -> Result<JsMintQuote> {
         let quote = self
             .inner
-            .mint_quote(amount.into())
+            .mint_quote(amount.into(), None)
             .await
             .map_err(into_err)?;
 
diff --git a/crates/cdk-cli/src/sub_commands/mint.rs b/crates/cdk-cli/src/sub_commands/mint.rs
index ac87ec1..46ce6a2 100644
--- a/crates/cdk-cli/src/sub_commands/mint.rs
+++ b/crates/cdk-cli/src/sub_commands/mint.rs
@@ -11,9 +11,10 @@ use cdk::wallet::multi_mint_wallet::WalletKey;
 use cdk::wallet::{MultiMintWallet, Wallet};
 use cdk::Amount;
 use clap::Args;
+use serde::{Deserialize, Serialize};
 use tokio::time::sleep;
 
-#[derive(Args)]
+#[derive(Args, Serialize, Deserialize)]
 pub struct MintSubCommand {
     /// Mint url
     mint_url: MintUrl,

@lollerfirst
Copy link
Contributor Author

lollerfirst commented Sep 11, 2024

@prusnak Don't we want the JS binding also use the description?

@prusnak
Copy link
Contributor

prusnak commented Sep 11, 2024

@prusnak Don't we want the JS binding also use the description?

Ah yes, of course we do :-)

@prusnak
Copy link
Contributor

prusnak commented Sep 11, 2024

I think we can also remove (now unused) MintMeltSettings from crates/cdk/src/cdk_lightning/mod.rs

Is that right @thesimplekid?

@thesimplekid
Copy link
Collaborator

I think we can also remove (now unused) MintMeltSettings from crates/cdk/src/cdk_lightning/mod.rs

Yes, don't believe there is any need for it now.

Copy link
Contributor

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

LGTM f4a0652

Copy link
Collaborator

@thesimplekid thesimplekid left a comment

Choose a reason for hiding this comment

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

ACK f4a0652

LGTM. Thanks for this

@thesimplekid thesimplekid merged commit 7e860c7 into cashubtc:main Sep 12, 2024
34 checks passed
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.

3 participants