Same with these consts, probably best to move them to some consts.ts file in ecash-lib

see below for a rationale

why I wouldn't move Wallet into ecash-lib: It uses ChronikClient, and that's a separate dependency

If we ever moved chronikclient into ecash-lib, we might also move Wallet

We'll need to change the design of this a bit—for postage, the wallet often doesn't have enough satoshis (even if fee = 0)

Instead, we should try to select sats if possible, and if there's not enough, simply mark the WalletAction as having missing sats (instead of throwing an error).

For tokens, this is different; they always have to be sufficient. But maybe it also makes sense to return to the user how many tokens are missing, so the user of this library can cleanly display it: "You are missing 50 MUSD to pay for this" (without having to catch and parse the error)

here, this will break any postage usage (see above)

The issue with this relates to the issue above; if the user has 0 of a token, we will fail to infer its token type (leading to a cryptic error). But it should return that we have insufficient tokens.

Instead, I think it's better to require the user to specify the tokenType in the action. It's part of the data we get from the BIP70 SLP Payment Protocol, and it's easy to look up if needed (via e.g. via the UTXOs already in the wallet, the /token endpoint as a fallback).

what a magnus opus

you might want to consider to move this out into its own file

This will fail in unexpected ways if we fail to infer the token type

We probably want utxo selection for postage to work differently than utxo selection for non-postage, since it's not about a shortage of sats so much as entirely disabling selection of fuel utxos. I think I can keep the prepareUtxos function if I use a param, but maybe preparePostageUtxos should be its own function.

For now, I have refactored to take gasless as an optional param (default false). If gasless, we do not look to add any fuel utxos. We do though, count satoshis from token utxos toward the required amount we need. I return postageSats as the sats missing to complete the tx.

need some more thought here. Throwing the error is I think the best way to handle the case of user calling .build().broadcast() and this request failing due to insufficient utxos -- makes sense, we tried to do this thing, we could not do it for this reason.

Alt, we could always return something. Have some kind of ActionReturn interface with a success key, and return something like

{
success: false,
reason: "Insufficient utxos"
utxosInfo: PreparedUtxosInfo
}

if .build() or .build().broadcast() fails due to utxo issues.

Refactored to parse this info from tokenActions instead of utxos

I thought would be able to simply drop the function entirely, but will probably still be a useful place to check things like valid / invalid combinations of token types (for example, the action of minting an SLP NFT will need special handling).

🙏

and this is still the "MVP" version ... no handling for NFTs, no handling for agora, no handling for chained txs. Writing this tho was indeed illuminating as to why this kind of lib could offer disproportionate impact to users.

Could have its own file but it is currently related to preparedUtxos, getActionTotals, getTokenType, etc ... which are also all directly related to the wallet. I'm opent to alt org suggestions but I'm not sure how to optimize the taxonomy.

still a bit of a WIP, not sure about the best way to handle this.

What does this indicate exactly? That we don't have sufficient tokens, or insufficient sats?

prepareUtxos returns an object that answers this. However now that prepareUtxos does not throw, if the user wants to build().broadcast() but can't because of a utxo issue ... we don't really know what to do about that.

I guess for the MVP, I could parse PreparedUtxosResult here and just throw if we don't have what we need. Then in the future we could decide how to handle the more granular cases.

did that for now -- lmk if that is not sufficiently extensible. I'm not really sure how to handle the postage stuff, but we should be sure that it isn't prevented by anything that gets in early.

In this case, this (MVP) implementation only supports intentional BURN txs. So if we cannot burn 100% of utxos, we cannot intentionally BURN without also having a SEND. This means the currently implementation of ecash-wallet only allows exact burn amounts for SLP tokens. This is probably better than what we currently have, where Cashtab allows specific amounts, but does so by allowing intentional burns in this special case.

We may fail to getTokenUtxosWithExactAtoms for a number of reasons, all of which are useful for the user to know. So, undefined ... while easier to handle, we lose a lot of info.

It's a spec-related error, so we throw a specific error so the user knows why the tx is off spec.

Separate to this function -- at the moment this error would just be thrown if someone is trying to .build().broadcast() and this is the reason it is not possible. We could alternatively add a SEND action and a change output -- but on SLP this would require either logic to support intentional burns when we have this case, or logic for a chained tx. And on ALP, it's nice for the user to decide if he wants to SEND and BURN intentionally (possible, specify both actions), or BURN only (possible and a different thing, in which case ... must have the right inputs, and this error informs if he does not).

😐

actually based on how the msg is constructed, this is what we want. we keep the space at the front to support listing multiple shortcomings

used a private method

...approach still seems kind of janky. just recently cleaned up a bug for this in Cashtab so important to test this. Can get complex at the borderline where we have enough sats in token utxos to cover output sats but not the fee -- which is surprisingly common in token wallets sending token txs.

added. still unsure best way to communicate this to a user.

We could have .build() return something with a {success: boolean}, but I'm not sure if that's better than current behavior (build() returns a BuiltTx or throws). If we did this, then we only throw on build().broadcast()

alt, we could add a checkBuild() function that does not throw and instead returns {success: boolean; builtTx: BuiltTx}

makes sense

Could you expand on this one? I'm not totally sure what we're looking to implement, still unfamiliar with how BIP70 is intended to work in practice. Mb easier to get into now that the other changes are taken care of (pending next update)