Skip to content

smite: move funding/commitment to channel_tx#91

Merged
morehouse merged 1 commit into
morehouse:masterfrom
NishantBansal2003:tx-builder
May 21, 2026
Merged

smite: move funding/commitment to channel_tx#91
morehouse merged 1 commit into
morehouse:masterfrom
NishantBansal2003:tx-builder

Conversation

@NishantBansal2003
Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Owner

@morehouse morehouse left a comment

Choose a reason for hiding this comment

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

Do you plan to implement a transaction builder object as the name suggests?

@NishantBansal2003
Copy link
Copy Markdown
Contributor Author

Do you plan to implement a transaction builder object as the name suggests?

Do we need that? I mean, I don't really see a need for a separate object that returns the constructed tx. I named it tx_builder because, in these modules, we are kind of building the tx, even though we are not explicitly returning it

If you think the name is confusing because of that, then I can change it to just transaction maybe?

@morehouse
Copy link
Copy Markdown
Owner

I'm not sure what the best name is, but it should probably have tx or transaction in it somewhere. "Builder" might imply some kind of builder pattern/object is contained within, which we currently don't have, and it also clashes a bit with the naming of future Build*Tx operations we'll implement. But having just transaction also collides with bitcoin::Transaction, and having just tx collides with bolt::tx_*.

So really it's all just bike-shedding. Whatever you think fits best works for me.

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
@NishantBansal2003 NishantBansal2003 changed the title smite: move funding/commitment to tx_builder smite: move funding/commitment to channel_tx May 21, 2026
Copy link
Copy Markdown
Owner

@morehouse morehouse left a comment

Choose a reason for hiding this comment

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

LGTM

@morehouse morehouse merged commit 269699e into morehouse:master May 21, 2026
5 checks passed
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