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

Improper Abstractions regarding TransactionDetails and UtxoNotification #200

Open
Sword-Smith opened this issue Oct 9, 2024 · 1 comment

Comments

@Sword-Smith
Copy link
Member

Struct TransactionDetails contains info needed for making a transaction in the context of interfacing with the client's state. (It now lives in a file name transaction_details.rs by the way.)

However, TransactionDetails has no field for PublicAnnouncements and perhaps it should. It does have a field for TxOutputs which have a UtxoNotification field, and if this field is set to the variant OnChain then the associated data is a PublicAnnouncement. With this architecture, it seems as though it is impossible to create transactions whose public announcements do anything other than notify receivers of UTXOs. This might be the case in the immediate future, but in the more distant future we definitely want to open up PublicAnnouncements to whatever the transaction initiator wants to say, even if that is not used for securely transmitting a ciphertext to someone. An example comes to mind: a disputed closure of a Lightning channel would require such a public announcement.

Here is a possible solution: variant UtxoNotification::OnChain contains a plaintext that is not encrypted yet, instead of the ciphertext cast to public announcement.

Co-authored-by: @aszepieniec

@dan-da
Copy link
Collaborator

dan-da commented Oct 9, 2024

create_transaction(), TransactionDetails and UtxoNotification are changed significantly in #185. TransactionDetails is replaced by TransactionParams.

I suggest that you complete the merge against master as-is, and then we can evaluate/discuss with regards to impl in #185. The current master is sort of an intermediate state. Fixing it (whatever that means) now would just create more merge headaches for #185.

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