Feature: FromUTXOs #45

tigh-latte · 2021-09-16T08:25:36Z

A utility func to attempt to abstract the funding flow of a tx away from the user. All the user has to do is provide an interator func.

Example usage:

if err := tx.FromUTXOs(ctx, fq, func(ctx context.Context, deficit uint64) ([]*bt.UTXO, error) {
		utxos := make([]*bt.UTXO, 0)
		for _, fund := range funds {
			deficit -= fund.Satoshis
			utxos = append(utxos, &bt.UTXO{fund.TxID, fund.Vout, fund.Script, fund.Satoshis})
			if deficit == 0 {
				break
			}
		}
		if deficit != 0 {
			return utxos, bt.ErrNoUTXO
		}
		return utxos, nil
	}); err != nil {
	return nil, errors.Wrap(err, "failed to fund transaction")
}

After completion, the tx is then ready for its change to be added, and ultimately signed.

codecov-commenter · 2021-09-16T08:30:55Z

Codecov Report

Merging #45 (54e6d75) into master (6e0ed1a) will increase coverage by 0.00%.
The diff coverage is 77.35%.

❗ Current head 54e6d75 differs from pull request most recent head a163144. Consider uploading reports for the commit a163144 to get more accurate results

@@           Coverage Diff           @@
##           master      #45   +/-   ##
=======================================
  Coverage   86.04%   86.05%           
=======================================
  Files          26       26           
  Lines        2817     2862   +45     
=======================================
+ Hits         2424     2463   +39     
- Misses        270      271    +1     
- Partials      123      128    +5

Impacted Files	Coverage Δ
txinput.go	`85.41% <77.27%> (-4.59%)`	⬇️
tx.go	`80.28% <77.77%> (+0.86%)`	⬆️
input.go	`84.00% <0.00%> (+2.66%)`	⬆️

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 6e0ed1a...a163144. Read the comment docs.

theflyingcodr · 2021-09-16T09:41:35Z

txinput.go

+
+// FundIteration contains information relating to the current fund. Its fields are intended
+// for use with tx.From(...).
+type FundIteration struct {


Rename this to just Fund I think - does that clash with anything else?

Nope, but I'm concious of bt.Fund being confused with tx.Input. What do you reckon?

Maybe it should just return a bt.Input, any reason it wouldn't?

Not really, it just means the the implementor has to build the Input themselves, being, parse the lockingscript into a *bscript.Script, and manually add the input tx id, instead of just being able to call the From helper function.

I'll change it over here now.

Added a NewInputFromhelper func to dodge this, let me know what you think.

theflyingcodr · 2021-09-16T09:43:58Z

txinput.go

+// containing relevant input information, and a bool informing if the retrieval was successful.
+//
+// It is expected that the boolean return value should return false once funds are depleted.
+type FundIteratorFunc func() (*FundIteration, bool)


As this will most likely be looking up funds from a data store, add ctx as an argument.

Would FundGetterFunc be a better name for this do you think as it explains more what it's doing, getting a single fund.

I thought since we're requesting n funds from a collection without any identifying args, leaving state management up to the implementor, iterator was better to describe it.

What do you think?

Aye the more I read it as FundGetter the more I agree actually

Yeah as each call will get a fund / input

theflyingcodr

just one small comment to fix, but looks good

theflyingcodr · 2021-09-16T14:42:44Z

txinput.go

 	"fmt"

 	"github.com/libsv/go-bk/crypto"

 	"github.com/libsv/go-bt/v2/bscript"
 )

+// ErrNoInput signals the InputGetterFunc has reached the end of its input.
+var ErrNoInput = errors.New("no remainings inputs")


should be 'remaining'

sorted there

jadwahab · 2021-09-18T21:51:41Z

txinput.go

+// NewInputFrom builds and returns a new input from the specified UTXO fields, using the default
+// finalised sequence number (0xFFFFFFFF). If you want a different nSeq, change it manually
+// afterwards.
+func NewInputFrom(prevTxID string, vout uint32, prevTxLockingScript string, satoshis uint64) (*Input, error) {


shouldn't these functions go into the inputs.go file instead of here? i think what we were going for was to try an minimise the number of ways users could do things. so in v1 the user had to create the input himself and then add that input to the tx, whereas with v2 the point is to use the .From() method to create the input as part of the tx straight away since having an input alone isn't much use is it? maybe we can keep this func but just put it in inputs.go and make in private? what do you guys think? @theflyingcodr @Tigh-Gherr ? that was the reasoning behind making tx.addInput private as well especially since it confused a couple devs into using it the wrong way in the past...

jadwahab · 2021-09-18T21:55:21Z

txinput.go

+//            return bt.NewInputFrom(utxos[i].TxID, utxo[i].Vout, utxos[i].Script, utxos[i].Satoshis), true
+//        }
+//    }())
+func (tx *Tx) FromInputs(ctx context.Context, fq *FeeQuote, next InputGetterFunc) (err error) {


ooh this is good shit, i like it 👍

jadwahab · 2021-09-18T22:02:19Z

txinput.go

+// relevant input information, and an err informing any retrieval errors.
+//
+// It is expected that bt.ErrNoInput will be returned once the input source is depleted.
+type InputGetterFunc func(ctx context.Context) (*Input, error)


i see this went thought a few iterations haha but i still think it may be able to go for 1 more iteration to make it little clearer - let's discuss on monday 👍

i really like where this is going actually, but i have a much better idea of how i would like this to work now and it follows the reasoning that we were discussing about potentially making inputs and outputs private that are used in a tx - since (unless someone can convince me otherwise) we will never have an input or output that isn't part of a tx and it just creates confusion, as we've seen previously, when separating them.

1st: we should probably return an array here instead of just one (input) since many times you will have an array of utxos to choose from and then we can add functionality to choose different utxos and stuff. plus of the utxogetter involved i/o then we'd want to minimise that and create a bulk call or something like that which we've done many times before.

2nd: it doesn't really feel accurate to say input getter since we're not getting the inputs, we're getting the utxos and creating the input... i think it would be much better to change this to UTXOGetterFunc and have it return an array of utxos and then we internally create the input from the utxo(s) and add them to the tx.

i'm still unsure whether we should still make the inputs/outputs private (ideally we could just leave them public to retain flexibility for users that know what they're doing but to remove all functions that imply separation between inputs/outputs and txs - for example the NewInput/Ouput funcs)...

thoughts?

add satoshis as a param and return array instead of only 1 element

create new data type: UTXO with the following 4 fields: prevTxID string, vout uint32, prevTxLockingScript string, satoshis uint64

refactor tx.From() to take UTXO instead of 4 params

rename InputGetterFunc to UTXOGetterFunc and return array of UTXOs

jadwahab · 2021-09-19T09:54:20Z

txinput.go

+// relevant input information, and an err informing any retrieval errors.
+//
+// It is expected that bt.ErrNoInput will be returned once the input source is depleted.
+type InputGetterFunc func(ctx context.Context) (*Input, error)


i really like where this is going actually, but i have a much better idea of how i would like this to work now and it follows the reasoning that we were discussing about potentially making inputs and outputs private that are used in a tx - since (unless someone can convince me otherwise) we will never have an input or output that isn't part of a tx and it just creates confusion, as we've seen previously, when separating them.

1st: we should probably return an array here instead of just one (input) since many times you will have an array of utxos to choose from and then we can add functionality to choose different utxos and stuff. plus of the utxogetter involved i/o then we'd want to minimise that and create a bulk call or something like that which we've done many times before.

2nd: it doesn't really feel accurate to say input getter since we're not getting the inputs, we're getting the utxos and creating the input... i think it would be much better to change this to UTXOGetterFunc and have it return an array of utxos and then we internally create the input from the utxo(s) and add them to the tx.

i'm still unsure whether we should still make the inputs/outputs private (ideally we could just leave them public to retain flexibility for users that know what they're doing but to remove all functions that imply separation between inputs/outputs and txs - for example the NewInput/Ouput funcs)...

thoughts?

jadwahab · 2021-09-21T11:09:05Z

txinput.go

 	for i, utxo := range pvsTx.Outputs {
 		utxoPkHASH160, err := utxo.LockingScript.PublicKeyHash()
 		if err != nil {
 			return err
 		}

 		if bytes.Equal(utxoPkHASH160, crypto.Hash160(matchPK)) {
-			if err := tx.From(pvsTx.TxID(), uint32(i), utxo.LockingScriptHexString(), utxo.Satoshis); err != nil {
+			if err := tx.From(&UTXO{


why not just have?

TxID: pvsTx.TxID(),

The TxID func hashes, reverses bytes then hex encodes every call. I figured there's no point in doing that work multiple times, being each iteration, since the result never changes, esp since the previous tx could have thousands of outputs.

smart 👍 (a comment there to explain would be great actually!)

jadwahab · 2021-09-21T14:28:35Z

utxo.go

+type UTXO struct {
+	TxID          string
+	Vout          uint32
+	LockingScript string


hmm you think this should be a *bscript.Script like the inputs and outputs? also the TxID be a byte array? https://github.com/libsv/go-bt/blob/master/input.go#L31

I'm happy with both of those, just as this is going to be used as params for tx.From, is there a reason the params for that function were both strings instead of []byte and *bscript.Script?

theflyingcodr

Just one suggestion around deficit name

txinput.go

theflyingcodr

LGTM

Tíghearnán Carroll added 2 commits September 16, 2021 09:19

Added utility AutoFund func to abstract funding flow

94bfa8d

updated doc string

644ae93

tigh-latte requested review from jadwahab and theflyingcodr September 16, 2021 08:25

mergify bot assigned tigh-latte Sep 16, 2021

tigh-latte changed the title ~~Feature: AutoFund~~ Feature: Fund Helper Sep 16, 2021

fixed error in autofund example

a67e50e

theflyingcodr requested changes Sep 16, 2021

View reviewed changes

Tíghearnán Carroll added 3 commits September 16, 2021 10:57

added context to fund iteration func

be5c4ba

renamed funditeration to fund

68fdc0b

renamed funditeratorfunc to fundgetterfunc

92a5633

theflyingcodr added the enhancement New feature or request label Sep 16, 2021

Tíghearnán Carroll added 7 commits September 16, 2021 12:27

fundgetterfunc now returns an error instead of a bool

0685253

fixed doc string

d053f1a

fixed doc string

e3c93b7

fixed linter and added test

e0fc927

fund getter replaced with input getter. added utility input builder func

7094e5b

removed all mention of funds

82e1bf0

updated example in doc string

8eff6b5

tigh-latte changed the title ~~Feature: Fund Helper~~ Feature: FromInputs Sep 16, 2021

theflyingcodr requested changes Sep 17, 2021

View reviewed changes

fixed type in error string

f50985d

jadwahab reviewed Sep 18, 2021

View reviewed changes

jadwahab requested changes Sep 19, 2021

View reviewed changes

Added utxo data type. changed tx.From signature. updated tests

bfe23db

jadwahab reviewed Sep 21, 2021

View reviewed changes

fixed deficit error and added tests

0d6ea41

removed println from tests

e2f01a1

tigh-latte changed the title ~~Feature: FromInputs~~ Feature: FromUTXOs Sep 21, 2021

Tíghearnán Carroll added 3 commits September 21, 2021 14:08

fixed doc string typos

cf29059

fixed more typos in docs

2db9190

fixed tests

5eb4ae6

jadwahab reviewed Sep 21, 2021

View reviewed changes

Tíghearnán Carroll added 3 commits September 21, 2021 15:35

added comment explaining precalculating prevTxID

85398dd

added FromUTXOs func. renamed old one to Fund

1a7d125

fixed doc string

73fd259

theflyingcodr requested changes Sep 22, 2021

View reviewed changes

txinput.go Show resolved Hide resolved

updated doc string to explain UTXOGetterFunc param

a163144

theflyingcodr added the work-in-progress If set, indicates the current PR is not ready for merging. Mergify will not auto merge label Sep 22, 2021

theflyingcodr approved these changes Sep 22, 2021

View reviewed changes

jadwahab approved these changes Sep 22, 2021

View reviewed changes

jadwahab removed the work-in-progress If set, indicates the current PR is not ready for merging. Mergify will not auto merge label Sep 22, 2021

mergify bot merged commit 695c030 into master Sep 22, 2021

mergify bot deleted the enhacement/auto-fund-tx branch September 22, 2021 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: FromUTXOs #45

Feature: FromUTXOs #45

tigh-latte commented Sep 16, 2021 •

edited

Loading

codecov-commenter commented Sep 16, 2021 •

edited

Loading

theflyingcodr Sep 16, 2021

tigh-latte Sep 16, 2021

theflyingcodr Sep 16, 2021

tigh-latte Sep 16, 2021

tigh-latte Sep 16, 2021

theflyingcodr Sep 16, 2021

theflyingcodr Sep 16, 2021

tigh-latte Sep 16, 2021

tigh-latte Sep 16, 2021

theflyingcodr Sep 16, 2021

theflyingcodr left a comment

theflyingcodr Sep 16, 2021

tigh-latte Sep 17, 2021

jadwahab Sep 18, 2021 •

edited

Loading

jadwahab Sep 18, 2021

jadwahab Sep 18, 2021

jadwahab Sep 19, 2021 •

edited

Loading

jadwahab Sep 20, 2021

jadwahab Sep 19, 2021 •

edited

Loading

jadwahab Sep 21, 2021

tigh-latte Sep 21, 2021 •

edited

Loading

jadwahab Sep 21, 2021 •

edited

Loading

jadwahab Sep 21, 2021

tigh-latte Sep 21, 2021

theflyingcodr left a comment

theflyingcodr left a comment

Feature: FromUTXOs #45

Feature: FromUTXOs #45

Conversation

tigh-latte commented Sep 16, 2021 • edited Loading

codecov-commenter commented Sep 16, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theflyingcodr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadwahab Sep 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadwahab Sep 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadwahab Sep 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigh-latte Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

jadwahab Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theflyingcodr left a comment

Choose a reason for hiding this comment

theflyingcodr left a comment

Choose a reason for hiding this comment

tigh-latte commented Sep 16, 2021 •

edited

Loading

codecov-commenter commented Sep 16, 2021 •

edited

Loading

jadwahab Sep 18, 2021 •

edited

Loading

jadwahab Sep 19, 2021 •

edited

Loading

jadwahab Sep 19, 2021 •

edited

Loading

tigh-latte Sep 21, 2021 •

edited

Loading

jadwahab Sep 21, 2021 •

edited

Loading