-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add Liquid API into web extension #2
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great seeing this documentation. It can definitely be useful in the future.
I glanced over it quickly and had a though on the wallet_sign_pset
function.
- `pset`: `String` - The PSET in base64 format | ||
- `inputs`: `Array<Input>` - The array on inputs to be signed | ||
|
||
- Input: `Object` | ||
- `index`: `Number` - The input index to be signed | ||
- `address`: `String` - The address associated with the input that requires signing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this come from a usecase/problem you faced in this way?
From an outside facing API I would assume that as a caller I do not need to know which input my wallet has to sign. My wallet should take care of this.
Internally, the wallet will then use such an API as it might need to sign multiple inputs.
Also, index
and address
seem to be redundant: if I tell the wallet to sign input 0
, it should know which address
(or key
) is capable of signing it.
I came across this, because we started using BDK and it uses this abstraction which seem to work well for our usecases:
wallet API:
https://docs.rs/bdk/0.3.0/bdk/wallet/struct.Wallet.html#method.sign
internal PSBT sign API:
https://docs.rs/bdk/0.3.0/bdk/wallet/signer/trait.Signer.html#tymethod.sign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This first iteration has been done to adapt it to what was already in place for Bitcoin/Ethereum chains, without much taking freedom. also I think this come from a point in time where PSBT in Bitcoin was not available, so the actual method was a lowish level signInput(index)
.
On that regard I totally agree with you, indeed in our internal Liquid JS Wallet library we have this signPset
method in the Identity interface which will try to sign any input with all the keys it has.
So totally agree in moving to that for Liquid and I will propose maybe to do the same for the Bitcoin part for signPsbt
cc/ @monokh
For example: We can retrieve 10 addresses using the `wallet_getAddresses` method: | ||
|
||
```js | ||
> await liquid.request({ method: 'wallet_getAddresses', params: [0, 10] }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the rationale behind using a JSON-RPC like model here? This interacts very poorly with strongly-typed languages. Personally, I would prefer APIs like:
await liquid.wallet.getAddresses(0, 10)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more a broader topic/discussion to have about the entire CAL itself. About the rationale I'm not the right person to answer to this, as per other comment I followed what was already in place, I may be wrong, but mostly the idea as far I can tell was to follow Ethereum/web3.js model, which in turns calls a JSONRPC server most of the time.
I agree also with this, I proposed offline indeed to have a typed binary serialization of messages (ie. Protocol Buffers) which would gives us typed specifications, self-generation of client/server stubs, backward&forward compatibility when changing/adding/removing message fields and also binary serialization ie. 3-5x faster that plain utf8 strings like JSONs.
Bonus point: self-generated API doc reference (since we are discussing in the documentation repo) :)
This PR adds the Liquid API to the documentation. Some things could change whene we finish the CAL liquid implementation, but this serves as a way to spark design dicussion and create a common ground into Liquid ecosystem