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

NEW API Planning & Discussion thread #1320

Open
Kukks opened this issue Feb 5, 2020 · 34 comments
Open

NEW API Planning & Discussion thread #1320

Kukks opened this issue Feb 5, 2020 · 34 comments

Comments

@Kukks
Copy link
Member

@Kukks Kukks commented Feb 5, 2020

This year we'll be focusing on making that new api come to life. The new api should not just be about replacing the compatible Bitpay API, but to also allow full usage of all of BTCPay's features, without its UI.

Before we can actually start opening PRs to add these APIs, we need to merge an authentication/authorization method, to which I currently propose #1262 ( there is also OpenId support partially there but I consider it deprecated)

In the meantime, I have a branch with various prototyping for new API endpoints that I've worked on for a while. https://github.com/Kukks/btcpayserver/tree/api-preview This branch contains endpoints to:

  • CRUD stores
  • CRUD rate rules for stores
  • CRUD bitpay api token for stores
  • CRUD payment methods for stores
  • CRUD Payment Requests

api dev doc preview: https://redocly.github.io/redoc/?url=https://gist.githubusercontent.com/Kukks/106d4c92058813204cad6dbefbe94058/raw/06a78f94bfc3922f3dd251c5a8df210df3de5c05/gistfile1.json

@dennisreimann

This comment has been minimized.

Copy link
Contributor

@dennisreimann dennisreimann commented Feb 5, 2020

I'd like to add a general health endpoint, which could be used to get the general status and high-level info like e.g. sync status. I'd be willing to work on this and help with the new API in general. :)

@Kixunil

This comment has been minimized.

Copy link

@Kixunil Kixunil commented Feb 5, 2020

Why is OpenId deprecated? It seems extremely useful to me. It's an open standard which we could use to connect with other applications. I actually started researching this topic yesterday and plan to make great use of it in my project.

@Kukks

This comment has been minimized.

Copy link
Member Author

@Kukks Kukks commented Feb 5, 2020

Why is OpenId deprecated? It seems extremely useful to me. It's an open standard which we could use to connect with other applications. I actually started researching this topic yesterday and plan to make great use of it in my project.

I stopped pushing for it after an extremely long and time consuming review process( which has not yet ended and finished being implemented after 1.5years). I created #1262 that covers most of the feature set we wanted with much less code and no external library dependencies.

@Kixunil

This comment has been minimized.

Copy link

@Kixunil Kixunil commented Feb 5, 2020

Hmm, I'm not sure if that can support my use case. What I want:

  • Just client - no OpenID/whatever provider needed
  • Allow external application to manage users
  • Allow external application to manage scopes
  • Do not bother the user with btcpayserver login page

Would it be possible to implement it? Maybe using nginx auth_request?

@Kukks

This comment has been minimized.

Copy link
Member Author

@Kukks Kukks commented Feb 5, 2020

@Kixunil

This comment has been minimized.

Copy link

@Kixunil Kixunil commented Feb 5, 2020

It looks like the authorization is interactive, I'd like it to be non-interactive. Sign in at external SSO provider and go directly to btcpayserver, without other confirmations or having to have the account there in the first place (it should be created during first access).

@eltNEG

This comment has been minimized.

Copy link

@eltNEG eltNEG commented Feb 7, 2020

I just got started with BTCPay and haven't looked at the code yet. But I would like to know if you would advise against building the API service as a separate docker service that can be attached to the current set of services. I would like to build the API service with golang as I am not familiar with c#

@Kukks

This comment has been minimized.

Copy link
Member Author

@Kukks Kukks commented Feb 7, 2020

I just got started with BTCPay and haven't looked at the code yet. But I would like to know if you would advise against building the API service as a separate docker service that can be attached to the current set of services. I would like to build the API service with golang as I am not familiar with c#

That wouldn't work since you need the internal C# services to expose the logic, data and events correctly.

@eltNEG

This comment has been minimized.

Copy link

@eltNEG eltNEG commented Feb 7, 2020

I just got started with BTCPay and haven't looked at the code yet. But I would like to know if you would advise against building the API service as a separate docker service that can be attached to the current set of services. I would like to build the API service with golang as I am not familiar with c#

That wouldn't work since you need the internal C# services to expose the logic, data and events correctly.

Ohk. Thanks

@Kixunil

This comment has been minimized.

Copy link

@Kixunil Kixunil commented Feb 8, 2020

I just remembered that there are ways to specify the API declaratively and then auto-generate the code from that in any language. Could this approach be used here? Would help a lot other projects, which might want to use different langs.

@Kukks

This comment has been minimized.

Copy link
Member Author

@Kukks Kukks commented Feb 8, 2020

I just remembered that there are ways to specify the API declaratively and then auto-generate the code from that in any language. Could this approach be used here? Would help a lot other projects, which might want to use different langs.

Yes, this is through Swagger/OpenAPI which I intend to use. :)

@pavlenex pavlenex added this to High Priority in BTCPay Server Project Feb 8, 2020
@Kixunil

This comment has been minimized.

Copy link

@Kixunil Kixunil commented Feb 8, 2020

I'm very happy to hear that! Also @pavlenex you just made my day putting it to high priority, as I have some wild ideas on how I could use this and want to implement soonish. :)

@Kukks

This comment has been minimized.

Copy link
Member Author

@Kukks Kukks commented Feb 8, 2020

soonish is optimistic :)

@eltNEG

This comment has been minimized.

Copy link

@eltNEG eltNEG commented Feb 8, 2020

I am also looking forward to it.

@Kixunil

This comment has been minimized.

Copy link

@Kixunil Kixunil commented Feb 8, 2020

For me it means month or two. :) But even if not, not a huge deal.

@Kukks

This comment has been minimized.

Copy link
Member Author

@Kukks Kukks commented Feb 11, 2020

I've published a draft of some of the APIs I'm playing around with here
https://app.swaggerhub.com/apis/btcpayserver/BTCPayServer/1.0.0

I'm still experimenting with the documentation generator and nothing is final

@Kixunil

This comment has been minimized.

Copy link

@Kixunil Kixunil commented Feb 11, 2020

I noticed amount in payment request is defined as decimal - is that float? Currency amounts must not be stored in floats because of accounting issues.

@Kukks

This comment has been minimized.

Copy link
Member Author

@Kukks Kukks commented Feb 11, 2020

I noticed amount in payment request is defined as decimal - is that float? Currency amounts must not be stored in floats because of accounting issues.

No, https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/floating-point-numeric-types

@Kixunil

This comment has been minimized.

Copy link

@Kixunil Kixunil commented Feb 11, 2020

Interesting, I wonder if there's native implementation of that in all langs via swagger.

@autotelik

This comment has been minimized.

Copy link

@autotelik autotelik commented Feb 12, 2020

swagger looks good so far. I'd like to be able to use BtcPay with multiple users, each with a store and wallet than can receive lightning payments. Basically an API that will enable micro payment use cases, supporting multiple providers of paid-for-content. Pretty much as someone else described here,

#1022

Thanks for all your amazing work.

@radWorx

This comment has been minimized.

Copy link
Contributor

@radWorx radWorx commented Feb 13, 2020

Is this meant to be a "Merchant" API dealing with Store/App specific calls? Or will it include a "Developer" API to interact with layers 1 & 2?

@Kukks

This comment has been minimized.

Copy link
Member Author

@Kukks Kukks commented Feb 13, 2020

Is this meant to be a "Merchant" API dealing with Store/App specific calls? Or will it include a "Developer" API to interact with layers 1 & 2?

This is mostly meant for BTCPay functionality.

@NicolasDorier

This comment has been minimized.

Copy link
Contributor

@NicolasDorier NicolasDorier commented Feb 13, 2020

posting that here, I was thinking about how a cli would looks like:

image

It may help to the design of the API.
I think we should start small.

@Kixunil

This comment has been minimized.

Copy link

@Kixunil Kixunil commented Feb 13, 2020

@NicolasDorier please don't pass token via command line, that is insecure.
I'd suggest looking into a file (default: ~/.btcpayserver/Main/.cookie), which can be overriden by --cookie-file
So the user can write AUTH="--cookie-file /some/file" making your example work securely.

Similarly, instead of --password, I'd suggest --password-file and maybe --generate-passwoord outputting a random password (trivial generating script: head -c 18 /dev/urandom | base64)

@Kukks

This comment has been minimized.

Copy link
Member Author

@Kukks Kukks commented Feb 13, 2020

@NicolasDorier please don't pass token via command line, that is insecure.
I'd suggest looking into a file (default: ~/.btcpayserver/Main/.cookie), which can be overriden by --cookie-file
So the user can write AUTH="--cookie-file /some/file" making your example work securely.

Similarly, instead of --password, I'd suggest --password-file and maybe --generate-passwoord outputting a random password (trivial generating script: head -c 18 /dev/urandom | base64)

This is not how it works and cannot work that way. The key needs to be provided as it depends on the user( a user can generate multiple keys with different permissions).

@Kixunil

This comment has been minimized.

Copy link

@Kixunil Kixunil commented Feb 13, 2020

OK, then don't use default, make --token-file a mandatory argument.

@Kukks

This comment has been minimized.

Copy link
Member Author

@Kukks Kukks commented Feb 13, 2020

It

OK, then don't use default, make --token-file a mandatory argument.

It is like that, nicolas just made a local var and added it in each call

@Kixunil

This comment has been minimized.

Copy link

@Kixunil Kixunil commented Feb 13, 2020

I understand. What I'm saying is store the secret into a file, not directly at command line. Having a default file was just an idea to make it more convenient for common cases, but not related to security.

@haarts

This comment has been minimized.

Copy link

@haarts haarts commented Feb 21, 2020

I noticed amount in payment request is defined as decimal - is that float? Currency amounts must not be stored in floats because of accounting issues.

No, https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/floating-point-numeric-types

While I cannot argue with the official docs I do believe the narrative changes when serializing this type to JSON. Different languages parse floats different because the JSON spec doesn't specific how this should be done. Sometimes you see floats in JSON documents as Strings because of this. I would argue that having only ints in any payload is preferred.

(for example this morning I had to write (double.parse(invoice['btcPaid']) * Coin.bitcoin().multiplier).round(), this is in Dart which doesn't have a decimal type, many languages don't.)

@haarts

This comment has been minimized.

Copy link

@haarts haarts commented Feb 21, 2020

I'm interested in this subject and if you don't mind I'll give my 2 cents here. Feel free to do with it whatever you want. I spent some time reading but certainly didn't touch upon every single endpoint. For example I don't know if it makes sense to update a payment request. Why not just create a new one and forgo all the complexities which come with this extra code path. Less is more.

I like what I'm seeing so far on https://app.swaggerhub.com/apis/btcpayserver/BTCPayServer/1.0.0.
I like the /api/v1 prefix.
I like plural resources (like stores) in the paths.
I like the sane nesting of resources (a store has a payment method).
I like the use of HTTP verbs. At some places a PUT could be replaced with PATCH. Common misconception.
I like the preview endpoints.

Authentication

You're proposing using API keys for authentication. Dropping the request signing as it is currently done. I do think it is important to understand what request signing brings to the table and what we will consequently lose if we don't do it.
IMO request signing prevents accidental leaks of secrets. With signing we don't send a secret so there's nothing to leak. With an API key we send the secret with each request. This is not a great problem when BTCPay Server is always serving ONLY TLS requests. But we can't guarantee that. People are free to install BTCPay Server anywhere. Also in a plain HTTP environment. Or perhaps they terminate the TLS connection at some load balancer and mess up their routing through a potentially insecure environment. We don't know. With request signing these problems do not exist. I would keep request signing. We're taking about a piece of software which is dealing with real money.

Unrelated; why not also have the GET request /api-keys/authorize under the /api/v1 path?

Misc

  • Let each field be of a single type (I'm looking at you exceptionStatus! This field can hold a bool or a string).
  • Each response should include all fields always. Don't omit/add fields depending on the situation.
  • Send a JSON error, not a HTML page whenever an exception occurs. Always. Also on 500s.
  • Use HTTP status codes. 201 is great. 302 is deprecated.
  • Tell the API consumer what went wrong and why. Error messages are really important.

This already is quite the wall of text. :)

@gerbz

This comment has been minimized.

Copy link

@gerbz gerbz commented Feb 24, 2020

Awesome - we've been wanting to allow BTCPay integration with BitLift! Seems like with the API merchants could add an API key and Store ID to their BitLift profile so we can generate invoices from their server when a customer buys their products?

I see in the docs we can POST to /api/v1/stores/{storeId}/payment-requests, but I dont see payment address info in the response. Am I misunderstanding how this would work? We would need all the payment info to integrate into our UI as oppose to iFraming or redirecting to an invoice.

@kemoantemo

This comment has been minimized.

Copy link

@kemoantemo kemoantemo commented Feb 28, 2020

Hi guys i think you remember that i was asking for a feature from the early days of our beloved project so i hope it will be added with the new api can we have a field that calculates all the confirmed balance that have been received? And if any new transaction became to the invoice it sends another api with the new confirmed balance so we don't need to handle the exception transactions manually and get the confirmed balance directly from api and use it in our programs it will be really great and sorry for my horrible English and i hope u can understand me ♥️💚

@Kukks

This comment has been minimized.

Copy link
Member Author

@Kukks Kukks commented Feb 28, 2020

Hi guys i think you remember that i was asking for a feature from the early days of our beloved project so i hope it will be added with the new api can we have a field that calculates all the confirmed balance that have been received? And if any new transaction became to the invoice it sends another api with the new confirmed balance so we don't need to handle the exception transactions manually and get the confirmed balance directly from api and use it in our programs it will be really great and sorry for my horrible English and i hope u can understand me ♥️💚

This has been available in the current invoice api for a long time. When you call get invoice through the bitpay api, there is a property called CryptoInfo which is an array of payments by each payment method, and each of them has a Payments list which has a Confirmed boolean.

@Kukks

This comment has been minimized.

Copy link
Member Author

@Kukks Kukks commented Mar 13, 2020

Awesome - we've been wanting to allow BTCPay integration with BitLift! Seems like with the API merchants could add an API key and Store ID to their BitLift profile so we can generate invoices from their server when a customer buys their products?

I see in the docs we can POST to /api/v1/stores/{storeId}/payment-requests, but I dont see payment address info in the response. Am I misunderstanding how this would work? We would need all the payment info to integrate into our UI as oppose to iFraming or redirecting to an invoice.

This was just a quick prototyping of how it would work, not final at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
BTCPay Server Project
  
High Priority
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.