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-4/NUT-5: Method-unit pair is object with settings #82

Merged
merged 2 commits into from
Mar 20, 2024

Conversation

callebtc
Copy link
Contributor

@callebtc callebtc commented Feb 7, 2024

This PR changes the "settings" for NUT-4/NUT-5 (minting and melting) that are shown in the /v1/info response.

The unit pairs are changed from an array ["method", "unit"] to an object that now also carries settings for the endpoints.

Example:

{
  "method": "bolt11",
  "unit": "sat",
  "min_amount": 0,
  "max_amount": 10000        
}

@ngutech21
Copy link
Collaborator

LGTM

Just one suggestion. I would like to rename method to payment_method, since this is more precise. If I read the term method in a Rest-Api my first thought is http-method.

@ngutech21
Copy link
Collaborator

Update: In the nut the struct is described as settings but in the json it's called method. Having method or payment_method twice feels redundant.

How about this:

 "4": {
      "settings": [
        {
          "payment_method": "bolt11",
          "unit": "sat",
          "min_amount": 0,
          "max_amount": 10000        
        }
      ],
      "disabled": false
    },

Copy link
Collaborator

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

I think it makes sense to have a more complex object for settings, and might even be a place where mints can add custom things that are useful for them/their users without breaking anything, because the wallet parsing could simply discard any field it does not know (I don't do that in my lib yet and the parsing errors out if there are extra fields on the response object but might turn that on if it's useful for people).

A few comments below.

04.md Show resolved Hide resolved
}
}
```
Here, `methods` is an array of arrays with `[method, unit]` pairs. `disabled` indicates whether this minting is disabled and the mint only allows melting ecash.

`MintMethodSetting` indicates supported `method` and `unit` pairs and additional settings of the mint. `disabled` indicates whether this minting is disabled.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I liked the wording you had above, and could combine both like so:

Here, methods is an array of MintMethodSetting. MintMethodSetting indicates supported method and unit pairs and additional settings of the mint. disabled indicates whether this minting is disabled.

05.md Show resolved Hide resolved
05.md Show resolved Hide resolved
@ngutech21
Copy link
Collaborator

Since cashu-ts is migrating to v1 api and this is only a small change, I think we should get this PR ready to merge before the wallets adopt the new api. Any thoughts on this? @gandlafbtc @callebtc

@gandlafbtc
Copy link
Collaborator

looks good to me

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.

Agree with tb's small points but overall LGTM

@callebtc
Copy link
Contributor Author

In the nut the struct is described as settings but in the json it's called method. Having method or payment_method twice feels redundant.

It feels appropriate to me that there is an array "methods" with entries that have a "method". Since we speak of "method-unit" pairs in the spec, I think the labels are ok or at least clear enough as is.

I have added all remaining suggestions and I think we are ready to merge this.

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.

5 participants