Page MenuHomePhabricator

[ecash-wallet] Add fluent API for building and broadcasting txs
Changes PlannedPublic

Authored by bytesofman on Wed, Mar 19, 04:27.

Details

Reviewers
tobias_ruck
Group Reviewers
Restricted Project
Summary

We want a powerful and user-friendly API that exposes commonly used steps in tx processing.

Here we initialize such an API with a method that supports non-token XEC txs.

  • We can get the utxos required to build an arbitary tx without actually building the tx by calling wallet.tx(<instructions>).utxos()
  • We can build a tx (and get info about it) without broadcasting it, by calling wallet.tx(<instructions>).build().fee() or wallet.tx(<instructions>).build().size(); or the actual TxBuilder with wallet.tx(<instructions>).build().tx
  • We can build and broadcast a tx with one call with await wallet.tx(<instructions>).build().broadcast()

    From here,
  • We should add a websocket so the user does not need to remember to call await wallet.sync() before sending every tx
  • Add support for token methods
  • Add support for chained txs, i.e. await wallet.tx(<instructions>).build().broadcast() should "just work" even if the instructions are asking to send more token outputs than a single tx can hold, or more XEC outputs than the tx broadcast limit of 100kb will tolerate.
Test Plan

npm test

integration tests:
build node in build dir, then, from bitcoin-abc/modules/ecash-wallet/, BUILD_DIR="${PWD}/../../build" npm run integration-tests

Review API, suggest different names, shapes of objects, anything that should go in now before we start to add more features

Diff Detail

Repository
rABC Bitcoin ABC
Branch
wallet-txs-regtest
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 32818
Build 65124: Build Diffecash-wallet-tests
Build 65123: arc lint + arc unit

Event Timeline

Improve comments, add tests for fee() and size() methods

tobias_ruck added inline comments.
modules/ecash-wallet/helpers.ts
42 ↗(On Diff #53181)

Careful, this will be incorrect as the signatory is missing, which will add bytes (eg the signature)

modules/ecash-wallet/package.json
7 ↗(On Diff #53181)

There’s something wrong with this. The unit tests should be in one folder (usually src) and the integration tests in another (usually tests), like it’s done for ecash-lib.

Then we can use the wildcard.

Otherwise we’ll have to add new tests here every time we add a new file.

modules/ecash-wallet/wallet.ts
42 ↗(On Diff #53181)

IMO you should change this to Recipient (or Output), and add sats and tokenId and isMintBaton, just like Output in my BIP70 diff.

And then we use that as unified interface for where to send tokens and the wallet just handles it.

Remember ALP allows multiple token IDs per send, I think we should enable this by default, that’s why it should be part of the output.

Also, keep in mind that we also need to enable attaching data in the OP_RETURN. For ALP/eMPP, this will be extra eMPP pushdata, and for non-eMPP, this could be OP_RETURN. Either way it would be an UInt8Array[] (which must be empty for SLP btw)

91 ↗(On Diff #53181)

Event is really not a good name IMO. TransactionsConfig (because you want this to be multiple ones). Recipients might work to. Or just SendConfig.

And I think there should be one unified config for sats only, tokens, and OP_RETURN data

93 ↗(On Diff #53181)

I think you just want one interface which handles all three—remember they can overlap in ALP, eg SENDs and MINTs and BURNs can be in 1 tx .

So you want the outputs as I describe above (with the mint baton Booleans), and the burn amount, and the OP_RETURN data.

  • outputs/recipients (sats and/or tokens, or mint baton)
  • burns (list of tokenId + burn amount)
  • opreturn data

And then the function figures out how to build the tx(s) based on that.

121 ↗(On Diff #53181)

Probably want to use Partial here instead of duplicating types

bytesofman added inline comments.
modules/ecash-wallet/helpers.ts
42 ↗(On Diff #53181)

ok -- added P2PKHSignatory and signing with dummy values

we could completely obscure all of the signing and estimating stuff and just build the tx with inputs signed by the wallet. But I think it's a useful static method to be able to just get the utxos for any arbitary tx, and we would have to convert signed inputs back to utxos to do this (well, I guess we could keep utxos as a class property of the built tx?. Mb that is a better approach...but we would still have to track them separately, we would still have to sign more than once. I like preserving the "just get the utxos sync" --> "just the signed tx and info about it, sync" --> "broadcast the tx, async"

modules/ecash-wallet/package.json
7 ↗(On Diff #53181)

good point, re-organized.

modules/ecash-wallet/wallet.ts
91 ↗(On Diff #53181)

went with somethin else but happy to bikeshed on this one a bit...the point of the class is to make things easier for devs but it's a tricky concept to describe succinctly, and I'm not really sure it's at all readily understandable to someone who hasn't been doing this for years

bytesofman marked 3 inline comments as done.

clean up + unify interfaces, use src/ and test/ directories to preserve test wildcards, add signatory for dummy sigs

bytesofman edited the test plan for this revision. (Show Details)

get rid of getEventUtxos static method, refactor so we get utxos from fluent API

tobias_ruck added inline comments.
modules/ecash-wallet/src/wallet.ts
39 ↗(On Diff #53214)

You can also make sats optional to indicate a leftover output, but not entirely sure.

Personally I think just one Output type is fine, not sure if we need another interface.

Also, for gasless payments, sats should be optional to indicate to the UTXO selector that we don’t need to add sats for the payment.

46 ↗(On Diff #53214)

BTW for GENESIS, we might need to have a placeholder for outputs that receive a GENESIS output (atoms or mint baton).

I see two options:

  • set tokenId to "GENESIS"
  • add a Boolean like isGenesis

And then the wallet tx builder knows how to build the genesis pushdata.

57 ↗(On Diff #53214)

I think this is adding too many terms. There’s config, instructions, action, send, transaction, event, all kinda meaning the same thing.

I think we have to pick one, and also a verb to interact with the wallet

I think the correct one is Action (ie an abstract description of what the user wants to do), and it indicates that it could potentially be multiple Transactions.

The wallet then maps Actions to Transactions.

Then, the wallet takes an Action and "builds" a list of Transactions.

The Transactions are then "broadcast" on the network (not sent, as this is already a thing in SLP/ALP).

69 ↗(On Diff #53214)

This is good. But I think we need to remove the output data from these (and move it into outputs), and also we need to add the token ID to each (except GENESIS).

There can be a mint, burn, send, genesis all in one tx (at least for ALP).

And these will most likely be commonplace in the future: You can burn tokens of one type and mint of another, which is kind of a „reminting“.

Or you can genesis and burn, where the genesis could be a sort of NFT receipt of the burn.

80 ↗(On Diff #53214)

Only these are needed.
The rest can be inferred from the outputs

94 ↗(On Diff #53214)
95 ↗(On Diff #53214)

don’t forget that there can be multiple burns per tx (possibly combined with a SEND)

98 ↗(On Diff #53214)
100 ↗(On Diff #53214)

I think the mint config should be encoded in the outputs.

So here we only need the genesisInfo

109 ↗(On Diff #53214)

Might be worth considering moving this into the Action, then it is self-contained.

118 ↗(On Diff #53214)

Oh wow there’s Required

Should it be exported though? Since it’s internal

224 ↗(On Diff #53214)

I think in the "long form" we have to have 4 functions in total. There ofc will be shorthands:

  • Wallet.calcTotal(Action): ActionTotal: Calculate the total token and sats amount
  • ActionTotal.prepareUtxos(): PreparedAction: Select UTXOs to fulfill the action. I like prepare because it has a nice adjective (select is a bit worse there)
  • PreparedAction.build(): WalletTxs: Build the actual txs (signed and everything).
  • WalletTxs.broadcast(): Promise<string[]>: Broadcast the txs and return the txids

This may seem a bit overkill, but trust me, it’ll be important.

Especially for gasless payments, you want to have a lot of fine grain control.

Especially prepareUtxos should give a lot of fine grained control, eg which SIGHASH to use.

232 ↗(On Diff #53214)

I think this function should

This revision now requires changes to proceed.Sat, Mar 22, 11:34
bytesofman marked 8 inline comments as done.

Update interfaces, add some intermediate steps, take sighash as a param

Pushing up this last round of feedback-based changes to support incremental review, but this is not ready to land yet

  • Confirm general path forward re: tokens and Action shape, imo we need to get a loose token API in this diff (make sure we aren't preventing anything about what we want to build), but we will need to really work out the token API as we add those features and confirm with regtest
  • Iterate on API changes with action and WalletTxs class; is this approach ok? Are we allowing enough granular control?
  • On confirming approach, get rid of what we don't need (i.e. with the approach up now, we no longer need the getRequiredUtxos function or the helpers.ts functions (e.g. getSignedDummyTx).
modules/ecash-wallet/src/wallet.ts
57 ↗(On Diff #53214)

nice yeah this makes sense / is simplest

69 ↗(On Diff #53214)

Difficult to get this right at the abstract level, i.e. before we are actually building these txs, getting the utxos for these transactions

High level -- should probably either

  1. define all token actions using only Output; design the interface of Output to accomodate this
  2. define all token actions where outputs must be calculated by token spec separately from Output in e.g. MintAction, BurnAction.

Imo there is a stronger case for 2.

For example if we have two genesis txs in Action ... we can't just have a bunch of outputs with no tokenId and isGenesis at the outputs key. We need to group the specific genesis outputs we want with each request. Would make sense to do this in distinct GenesisAction objects.

For this diff, I think we need a reasonably-acceptable framework + tentative dev direction. Will need to focus on the specific logic in diffs that add + test this functionality.

80 ↗(On Diff #53214)

If we go this way, do we need MintAction at all? could be a useful interface for GenesisAction but I don't see why we need it as a tokenAction; mint outputs could be defined by isMintBaton and tokenId in Output ....

With this approach, we would also need some way in Output to distinguish between MINT outputs and SEND outputs.

It seems pretty complicated if we specify Outputs for MINT actions in the outputs: key but then also have to "color" these outputs with a MintAction. Want to limit as much as possible the need for a spec just to create the input for ecash-wallet txs.

As above tho, it's hard to get this right in a diff that is not building and testing mint txs. For now we should get a loose placeholder and general agreement on the dev direction, likely to change as we implement.

100 ↗(On Diff #53214)

what if we have more than one GenesisAction? How do we know which isGenesis outputs belong to which GenesisAction?

imo it makes sense to define action-specific genesis outputs in an action interface. Or we could make a rule about only one genesis action allowed.

109 ↗(On Diff #53214)

yes, this is simpler. Since Action will need to be some kind of Interface anyway, might as well add config there as keys. Can still get the desired behavior of "if these keys are not specified, ecash-wallet will use sensible default values".