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

Initial project layout, errors, and utils #8

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

tnull
Copy link
Collaborator

@tnull tnull commented Sep 1, 2022

This PR provides the initial project layout, initial error types, and basic hex and logger utils.

@tnull tnull force-pushed the 2022-09-initial-project-structure branch from a8ca461 to 74142a7 Compare September 1, 2022 07:09
@tnull tnull changed the title Initial project layout and utils Initial project layout, errors, and utils Sep 1, 2022
@tnull tnull force-pushed the 2022-09-initial-project-structure branch from d4a0e9d to 3f25117 Compare September 8, 2022 10:37
@tnull tnull force-pushed the 2022-09-initial-project-structure branch from 3f25117 to d384ef9 Compare September 12, 2022 11:49
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated
/// better safe than sorry..
FundingTxNonWitnessOuputSpend,
/// The funding transaction could not be finalized.
FundingTxNotFinalized,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why couldnt we finalize it? What does this mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, it would fail to be finalized if it includes inputs that cannot be satisfied at the current time, e.g., due to being timelocked. Replaced this with a more generic FundingTxCreationFailed error.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But shouldn't that be handled by not selecting those inputs? Presumably the error should just be "we don't have enough (available) funds (right now)", no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, IIUC they go through all previously unsigned inputs, try to derive the right wallet descriptor, and sign them. So in our case you're probably right, but while the word 'finalize' appears quite a lot it's kinda poorly documented what it actually does/why it's an extra step. I'll try to clarify with the BDK folks and will probably open an issue..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, it seems that while individual inputs are signed before, the witness is just created and added in the finalization step. I'm still not quite sure when this step would ever fail in our case, since not having enough funds should already become apparent during input selection, i.e., during tx_builder.finish(). So, I'll probably just capture all that via the FundingTxCreationFailed error.

src/error.rs Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
src/hex_utils.rs Outdated Show resolved Hide resolved
src/hex_utils.rs Outdated Show resolved Hide resolved
src/logger.rs Show resolved Hide resolved
@TheBlueMatt
Copy link

LGTM, feel free to squash + merge.

@tnull tnull force-pushed the 2022-09-initial-project-structure branch from 8c15092 to 1a81027 Compare September 19, 2022 16:47
@tnull tnull merged commit b646a57 into lightningdevkit:main Sep 19, 2022
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

Successfully merging this pull request may close these issues.

2 participants