Skip to content

Rename DummyTxOutData::new_with_amount to new#29

Merged
arminsabouri merged 1 commit intopayjoin:masterfrom
bc1cindy:refactor-dummytxoutdata-constructors
Mar 31, 2026
Merged

Rename DummyTxOutData::new_with_amount to new#29
arminsabouri merged 1 commit intopayjoin:masterfrom
bc1cindy:refactor-dummytxoutdata-constructors

Conversation

@bc1cindy
Copy link
Copy Markdown
Contributor

rename DummyTxOutData::new_with_amount to new for the most common constructor and align with DummyTxData

@arminsabouri
Copy link
Copy Markdown
Collaborator

@bc1cindy needs a rebase

@bc1cindy bc1cindy force-pushed the refactor-dummytxoutdata-constructors branch 2 times, most recently from 77ea704 to aa56999 Compare March 30, 2026 15:45
@bc1cindy
Copy link
Copy Markdown
Contributor Author

@bc1cindy needs a rebase

rebased! @arminsabouri

Copy link
Copy Markdown
Collaborator

@arminsabouri arminsabouri left a comment

Choose a reason for hiding this comment

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

Just a suggestion to clean up some boilerplate. Lmk if it makes sense.

Comment on lines +46 to +54
DummyTxOutData::new(100, 0),
DummyTxOutData::new(100, 1),
DummyTxOutData::new(100, 2),
DummyTxOutData::new(200, 3),
DummyTxOutData::new(200, 4),
DummyTxOutData::new(200, 5),
DummyTxOutData::new(300, 6),
DummyTxOutData::new(300, 7),
DummyTxOutData::new(300, 8),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see new is mostly being used as a element of a vec. Would it make more sense to refactor DummyTxData::new_with_outputs to take a vec of amounts. That way we dont have to explictly pass in vout -- it can be calculated by new_with_outputs using enumurate()
e.g

DummyTxData::new_with_outputs(vec![ 100, 100, 100, 200, 200, ..., 300])

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

makes sense, I'll do it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

cleaning time

I kept new_with_outputs intact because same_address.rs needs to pass explicit scripts via new_with_script, but added DummyTxData::new_with_amounts

@bc1cindy bc1cindy force-pushed the refactor-dummytxoutdata-constructors branch from aa56999 to 07d666f Compare March 30, 2026 23:43
Copy link
Copy Markdown
Collaborator

@arminsabouri arminsabouri left a comment

Choose a reason for hiding this comment

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

Great clean up. Seems a bit inconsistent. Someplaces still using new() when I think they can use new_with_amounts?

Comment thread src/crates/heuristics/src/ast/uih.rs Outdated
DummyTxOutData::new_with_amount(250, 0),
DummyTxOutData::new_with_amount(40, 1),
],
vec![DummyTxOutData::new(250, 0), DummyTxOutData::new(40, 1)],
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Anything preventing us from using new_with_amounts here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

new_with_amounts creates funding txs with no inputs, so I created new_with_spent for spending txs.

also added comments to be clear

@bc1cindy bc1cindy force-pushed the refactor-dummytxoutdata-constructors branch from 07d666f to 2792eb9 Compare March 31, 2026 15:18
Copy link
Copy Markdown
Collaborator

@arminsabouri arminsabouri left a comment

Choose a reason for hiding this comment

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

re-ACK

@arminsabouri arminsabouri merged commit f464da3 into payjoin:master Mar 31, 2026
3 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