-
Notifications
You must be signed in to change notification settings - Fork 47
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 DecodeSidecarTicket RPC #285
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.
PR LGTM, just have one question regarding encoding.
rpcserver.go
Outdated
OfferLeaseDurationBlocks: t.Offer.LeaseDurationBlocks, | ||
OfferSignPubkey: serializePubKey(t.Offer.SignPubKey), | ||
OfferAuto: t.Offer.Auto, | ||
ExecutionPendingChannelId: nil, |
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.
nit: no need to nil
ExecutionPendingChannel
.
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.
Yes, fixed.
rpcserver.go
Outdated
} | ||
|
||
if t.Execution != nil { | ||
resp.ExecutionPendingChannelId = t.Execution.PendingChannelID[:] |
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.
Do we want to return raw byte arrays here and for OrderBidNonce
and ID
? I see in the next commit we use printJson
but perhaps we could use a hex encoding for these byte buffers instead?
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.
We're using github.com/lightninglabs/protobuf-hex-display
for the printRespJSON()
, so it will be printed on the command line as hex. I actually prefer having things as raw bytes on the protobuf level. That does make it a bit more difficult to use with REST since it'll be base64 by default. But using hex encoded strings just because of that feels kind of inefficient. Not sure about this tbh. What's your take, @Roasbeef?
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 that protobuf-hex-display
takes care of this. As for REST I'd say base64 is probably more convenient to use than hex encoding.
1e267bb
to
79bae1a
Compare
To make it possible to decode a sidecar ticket through RPC instead of only relying on the output of the CLI, we add the DecodeSidecarTicket RPC method to the trader daemon.
79bae1a
to
3d015ec
Compare
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.
LGTM 🏊♀️
MultiSigPubKey: testPubKey2, | ||
NodePubKey: testPubKey, | ||
MultiSigPubKey: testPubKey2, | ||
MultiSigKeyIndex: 77, |
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.
👍
sidecar channels 2/3: Allow sidecar tickets to be specified on bid orders
Fixes #263.
Adds the
DecodeSidecarTicket
RPC.Pull Request Checklist
used.