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

Drop From<Vec<u8>> for UnsignedBolt12Invoice #3373

Open
TheBlueMatt opened this issue Oct 16, 2024 · 5 comments
Open

Drop From<Vec<u8>> for UnsignedBolt12Invoice #3373

TheBlueMatt opened this issue Oct 16, 2024 · 5 comments
Assignees

Comments

@TheBlueMatt
Copy link
Collaborator

...and maybe others. They only write the invoice request's bytes out, not the fields for the Invoice itself. We shoould probably rename UnsignedBolt12Invoice::bytes to invreq_bytes (and possibly similarly in invreq?) to avoid this confusion in the future.

@TheBlueMatt TheBlueMatt added this to the 0.1 milestone Oct 16, 2024
@TheBlueMatt
Copy link
Collaborator Author

TheBlueMatt commented Oct 16, 2024

While we're at it we might want to consider dropping the From<Vec<u8>> stuff for the unsigned types since they're not used, have a similar issue, and don't actually take ownership of the Vec anyway. When we're not taking ownership of the vec we should just deserialize as Readable, IMO.

@jkczyz
Copy link
Contributor

jkczyz commented Oct 16, 2024

...and maybe others. They only write the invoice request's bytes out, not the fields for the Invoice itself. We shoould probably rename UnsignedBolt12Invoice::bytes to invreq_bytes (and possibly similarly in invreq?) to avoid this confusion in the future.

It looks right to me. The constructor takes invreq_bytes by reference and contents by value, forming bytes from them to use in UnsignedBolt12Invoice.

@TheBlueMatt TheBlueMatt removed this from the 0.1 milestone Oct 16, 2024
@TheBlueMatt
Copy link
Collaborator Author

Ah, apologies, I was confused. In any case, I do think we should drop the From<Vec<u8>> impl because we aren't actually really being built from a set of bytes (it doesn't make sense for an unsigned object, its bytes arent final).

@TheBlueMatt TheBlueMatt changed the title Readable/Writeable for UnsignedBolt12Invoice are wrong Drop From<Vec<u8>> for UnsignedBolt12Invoice Oct 16, 2024
@jkczyz jkczyz self-assigned this Oct 16, 2024
@TheBlueMatt
Copy link
Collaborator Author

TheBlueMatt commented Oct 16, 2024

Alternatively, we could use the bytes and pipe them through to the UnsignedBolt12Invoice instead of deserializing and then reserializing the invoice request fields. Oh guess we can't do that cause then we'd be signing things we don't understand..should we be failing to deserialize unsignedbolt12invoices that have unknown fields?

@jkczyz
Copy link
Contributor

jkczyz commented Oct 24, 2024

Ah, apologies, I was confused. In any case, I do think we should drop the From<Vec<u8>> impl because we aren't actually really being built from a set of bytes (it doesn't make sense for an unsigned object, its bytes arent final).

Hmmm... I don't think this is the case because ParsedMessage::try_from builds the message from the bytes. IIRC, we added the From<Vec<u8>> implementation since a remote NodeSigner may need to (de-)serialize the bytes.

Alternatively, we could use the bytes and pipe them through to the UnsignedBolt12Invoice instead of deserializing and then reserializing the invoice request fields. Oh guess we can't do that cause then we'd be signing things we don't understand..should we be failing to deserialize unsignedbolt12invoices that have unknown fields?

Since it goes through ParsedMessage, we'd fail to deserialize any unknown even fields.

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