broadcastTx decodes the hex again anyway

if I use the EccDummy approach (and therefore no ecc) --- how should the inputs be refactored so that the size is still correctly considered?

ref line 219, above

dang...so, this would be better.

However, all of the chronik mocks are set with raw hex, which is much easier to manage in the tests than UINT8 arrays (raw hex is easy to copy paste into electrum abc to check, easy to console.log when testing new txs).

Since Cashtab isn't really machine gunning txs I think this isn't worth changing.

shouldn't this be based on minFee from the settings?
also, any reason to check this here?

makes sense, it's cheap enough anyway so that should be fine

to estimate the tx size, you don't need the actual private key here, you can just set a dummy, they will result in the same estimation.

if you use the EccDummy here, the derive will be free

Same here, you can just put a dummy P2PKH script

Probably not really a worthwhile check, esp in this function which can only be called using a fee param that is only dev-set. I added this in this diff to preserve the behavior of a unit test from before this migration, which was throwing an error from ecash-coinselect.

removed.

this unit test was confirming expected behavior of ecash-coinselect, so kind of "extra" even before this implementation.

In general, we do not need to be running a min fee check every time a tx is built (when the min fee param cannot be set by the user).

Cashtab is now creating raw txs that are schnorr signed instead of ECDSA signed. So, the raw hex is different than before.

the way chronik-client works, rawtxs are broadcast with chronik.broadcastTx(rawTx)

if the tx is successfully broadcast, then chronik-client returns the txid.

mock-chronik-client is dumb. you just tell it what call to return for an expected call and it returns it. In these tests, we are telling mock-chronik-client "you should expect a chronik.broadcastTx call for this particular hex string, and, when you receive it, please return this txid, to mock a successfully broadcast tx in prod."

If we do not update the rawtx in the mock

• we set mock-chronik-client with ECDSA_rawtx
• cashtab builds a rawtx with schnorr, and schnorr_rawtx !== ECDSA_rawtx
• the function tries to broadcast schnorr_rawtx but the mock has not been set for this, so it returns a 'not found' error

So, the unit test(s) and integration tests in Cashtab are really testing that Cashtab is building the expected rawtx. We don't really test that chronik successfully broadcasts the txs. But, we need this mock bc Cashtab is set up to actually broadcast txs, which are different every time.

In the integration test we can then confirm that we show a notification correctly (since we have set the txid response).

This diff changes every tx that Cashtab builds from ECDSA to schnorr, so all of the mocks need to be changed.

here's the tx Cashtab built using utxo-lib

Here's the tx Cashtab is building now, using ecash-lib

good point. for now will leave it as is

• have to modify the outputs conditionally already (may need to do this with inputs at some point in the future)
• not a function that is called frequently (can only be called by manual user action)
• functional / readable

It is a bit strange. I think it's the best way, at least for now.

• All txs are XEC txs, so all txs will need to determine eCash utxos
• Token txs all have different rules. Is this SLP1 or 2? NFT? genesis? Send? burn? ALP? If NFT, is this a fan-out tx? etc.

We handle the token input selection in a variety of functions that do not play nicely with each other as they all have different rules. See slpv1/index.js

There is probably an opportunity to clean it up a bit by calling something like getTokenInputs(tokenType, txType, options) in this function. However, I think this should be implemented in its own diff after Cashtab is fully on schnorr.

I have also for example left out slp-mdm deprecation in this diff -- ecash-lib handles token OP_RETURN creation better, but there are so many call sites for this that imo it would be overloading this diff to handle it here.

Getting this diff in gives us a baseline of expected schnorr-signed rawtxs. Then, when we swap out slp-mdm for ecash-lib, we can check the implementation is good as we should then expect the same rawTxs.

imo this introduces too much complexity. The sign function in ecash-lib is already making estimates with a dummy tx...so now we make an estimate before the estimate?

What if we estimate here assuming a change output, think we need another output to cover change, but we actually could have done the tx with no change (which ecash-lib will already figure out)?

It should be handled better, agreed. Either adding utxo selection methods to ecash-lib or making this part of the tx builder. But if it will be a bit unfinished here no matter what, I don't know if we get enough impact from adding another estimator here as a sort of scaffolding, planning to drop it later anyway.

It is probably "less expensive" to not sign twice, but this is not something that will be automated and happening all the time in Cashtab. Weird edge case where we sign more than once at all (we only build the tx once input value exceeds what we need for output, so would only get this where we happen to be "just over the line" and do have enough to cover outputs but do not have enough to cover outputs + fee).

potentially tho I just don't really understand how to do this and making the estimate is easier than I realize.

There's other errors that may be thrown here (e.g. missing SignData), so IMO you should check if the error starts with "Insufficient input value", and let it bubble up otherwise.

good catch, updated

yes. At the moment, it can only be of length 0 or 1. But var name should be plural as it is an array and may be expanded later.

this makes me wonder if you either apply this transformation to all items in the array, or make it scriptOutput (maybe better opreturnOutput ?) and optional (in ts: TxOutput | undefined), instead of a 0 or 1 sized array

another unfortunate JS mgmt thing.

ran this to update

rm rf node_modules/
rm package-lock.json
npm i

true, imo it should be added to ecash-lib.

Even then, we might not really need it. It's only necessary in this diff because Cashtab stores private keys in WIF format, since this is what utxo-lib needed to sign with. I did not want to combine a wallet migration with this diff.

As utxo-lib deprecation is completed, would make sense to migrate private key storage to ecash-lib expectation, and then lose this dependency.

For now -- it's only 4kb, very simple lib that is basically just a function. Simplifies implementing ecash-lib.

https://github.com/bitcoinjs/wif/blob/master/index.js

🤔

I can tell that this test does not get to the point of making this tx due to the thrown error, but I am not sure why this tx was originally included at all.

oh ok. Looking into it -- some of the thrown error tests will expect a tx to be built. Others don't

For the ones that do not -- this tx is just copy pasted as a dummy tx. You can see that this is the same as the preceding test vector.

Updated so dummy tx is more clear.