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

Split up the API to smaller parts #9

Closed
Kixunil opened this issue Dec 18, 2018 · 4 comments
Closed

Split up the API to smaller parts #9

Kixunil opened this issue Dec 18, 2018 · 4 comments

Comments

@Kixunil
Copy link

Kixunil commented Dec 18, 2018

Currently, the API allows one to either implement everything or nothing at all. This goes against interface segregation principle. Some issues this leads to:

  • Wallets like Eclair that don't support receiving are unusable since they can't possibly implement creating invoices.
  • The API encourages one to request node identity, which harms privacy. For example Lightning Spin requires knowing node info, but for the basic operations (deposit, withdrawal), this is not needed. If one rejects providing node info, it falls back to not using the extension at all.
  • A developer of an extension can't provide features gradually, beginning with some basic (like pay invoice) and only then continuing with more advanced stuff (message signing and verification). This is something I'm interested in, as I have desire to gradually develop some extension (more Qubes OS-friendly version of Joule).
  • Even if a website doesn't require to know node info, an extension may require user to allow giving it. Cautious user would reject such request without knowing there's no actual harm in accepting it. By splitting it, a website can convey "no private info required" more cleanly.
  • A user might be afraid of entering their admin macaroon into an extension but would like to benefit from being able to receive payments by providing invoice macaroon. (Or equivalent access token for non-LND nodes.) By forcing extensions into implementing whole API, this use case is impossible to address.

My suggestion would be to split the interface into these parts:

  • Paying invoices
  • Making invoices
  • Node info
  • Message signing and verification

The specification should not require extensions to implement every part and should recommend websites to handle missing features by disabling relevant features of the website instead of ignoring the API completely (e.g. Lightning Spin could make use of invoices APIs and ignore the rest, if it isn't provided).

Further, this allows extending the interface more cleanly in the future (e.g. adding channel manipulation API, on-chain API, etc), as new additions would be separate from existing and still optional, thus not breaking any old implementations. This is also a very good reason for recommending granular handling of errors.

@wbobeirne
Copy link
Member

This is a totally valid point, however nothing precludes you from implementing a provider that throws on certain methods. In fact, that's what I do now in Joule (and was doing before I had implemented makeInvoice). Perhaps providers should have a better way of indicating what methods they support, but for now you can just throw. All methods are promises anyway, so the application's error handler should catch it.

Lightning Spin requires knowing node info

This actually isn't the case. Joule gives that permission by default right now, but nothing would stop me from allowing the user to not provide this information, and just throwing on getInfo.

I agree that there should probably be some kind of OAuth style permission requesting in enable calls, I just didn't want to draft up what that would look like without having compelling use cases for it yet.

@Kixunil
Copy link
Author

Kixunil commented Dec 19, 2018

Of course, throwing is an option, the question is whether websites will handle it correctly. I'm not experienced in web development, so maybe the culture of web is good enough that this is non-issue and in that case, sorry for bothering.

What I found when programming is that rather than throwing/returning errors, it's better to no provide the function at all, so that the user of API is forced to check it. Of course this is implying strict null checking.

If you choose to recommend throw and catch approach, please at least document what can throw and when, as I wasn't sure from just looking at the interface documentation. (But maybe "everything can throw, you must catch everything" is implied in JS and it's just me not knowing JS enough?)

@wbobeirne
Copy link
Member

Typically promises are expected to throw due to the nature of them usually dealing with network requests, file system access, etc. But you're absolutely right, this could be more explicit.

Each of the methods are going to need much more documentation and examples at some point, so I'll definitely be sure to outline the errors they can throw (and ideally standardize them into actual error classes instead of them all being generic Errors.)

I'm going to close this issue and open two new ones with the things above.

@Kixunil
Copy link
Author

Kixunil commented Dec 19, 2018

OK, I've also opened an issue in Joule for addressing permissions. Thank you for explanation and consideration!

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

No branches or pull requests

2 participants