Skip to content

added ability to define which utxos to use #61

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 22, 2020
Merged

added ability to define which utxos to use #61

merged 6 commits into from
May 22, 2020

Conversation

@maikelmclauflin
Copy link
Contributor

@maikelmclauflin maikelmclauflin commented Feb 20, 2020

fixes #44

@codecov-io
Copy link

codecov-io commented Feb 20, 2020 •
edited
Loading

Codecov Report

Merging #61 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
+ Coverage   93.56%   93.61%   +0.04%     
==========================================
  Files          33       33              
  Lines        1835     1848      +13     
  Branches      255      257       +2     
==========================================
+ Hits         1717     1730      +13     
  Misses        112      112              
  Partials        6        6
Impacted Files Coverage Δ
packages/cashscript/src/interfaces.ts 100% <ø> (ø) ⬆️
packages/cashscript/src/Transaction.ts 89.36% <100%> (+0.23%) ⬆️
packages/cashscript/src/Instance.ts 100% <100%> (ø) ⬆️
packages/cashscript/src/util.ts 97.4% <100%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a01866c...9498caa. Read the comment docs.

rkalis
rkalis requested changes Feb 21, 2020
View reviewed changes
Copy link
Member

@rkalis rkalis left a comment

Choose a reason for hiding this comment

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

Awesome work so far @maikelmclauflin, thanks a lot for picking up this issue!
I highlighted a few points in the review, but overall really nice work.

packages/cashscript/src/Transaction.ts Outdated
_utxos?: Utxo[],
): Promise<{ inputs: Utxo[], outputs: OutputForBuilder[] }> {
const { utxos } = await this.bitbox.Address.utxo(this.address) as AddressUtxoResult;
let utxos = _utxos;
Copy link
Member

@rkalis rkalis Feb 21, 2020

Choose a reason for hiding this comment

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

So when someone passes in their own list of inputs, all of them should be used. I think that with this code change, it only replaces the "pool" of possible UTXOs, but then it still performs the UTXO selection algorithm below. This means that some of the manual inputs might get excluded due to the selection process.

So instead, if a manual list of utxos is provided, this should completely replace the "selection process". But it should still add a change output where necessary, and it should also check that the provided UTXOs are enough to cover the transaction. Does that make sense?

maikelmclauflin and others added 5 commits March 11, 2020 12:04
@maikelmclauflin @rkalis
added ability to define which utxos to use
29a71cd
@maikelmclauflin @rkalis
finds utxo that was passed into send
387a25a
@maikelmclauflin @rkalis
linted and adjusted tes
5a631da
@maikelmclauflin @rkalis
incorporated changes
4ae9242
@rkalis
Changes to manual utxo inputs tests
2024565 
- Allow unconfirmed UTXOs to be used
- Replace `total` with `amount` for "happy case" test
- Add type annotation for gatherUtxos parameter
@rkalis rkalis force-pushed the manual-utxo-selection branch from 5797d17 to 9498caa Compare March 11, 2020 11:08
@rkalis rkalis force-pushed the manual-utxo-selection branch from 9498caa to d38a9c3 Compare March 26, 2020 10:55
@rkalis
Copy link
Member

rkalis commented May 22, 2020

I'm planning to change the Transaction send interface around, so I'll merge this change in, and make some changes to fit in the new interface.

@rkalis rkalis merged commit 107707f into CashScript:master May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rkalis rkalis rkalis requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Manual UTXO selection

3 participants

@maikelmclauflin @codecov-io @rkalis