Page MenuHomePhabricator
Differential D18867

[ecash-wallet, ecash-lib] Support chained XEC txs for XEC-only actions that would otherwise exceed the broadcast size limit
ClosedPublic
Actions

Authored by bytesofman on Tue, Nov 4, 22:13.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCe60f96ad338e: [ecash-wallet, ecash-lib] Support chained XEC txs for XEC-only actions that…
Summary

Support automatic creation of "chained" txs to split up XEC-only Actions that would exceed MAX_TX_SERSIZE if built in a single tx.

This diff allows app developers to send XEC to an arbitrary number of recipients. It is useful for app payment use cases where the number of recipients of a payment may not be known beforehand. For example, an airdrop of XEC to token holders, where the number of token holders and their holdings changes over time, but payments should occur at fixed intervals.

We do not run into this constraint often. But the existing airdrop calculator in Cashtab should properly use this method, as attempting an XEC airdrop to holdres of any token with a few thousand holders would otherwise cause a MAX_TX_SERSIZE error.

Test Plan

npm test, CI

Diff Detail

Event Timeline

bytesofman created this revision.Tue, Nov 4, 22:13
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Nov 4, 22:13
bytesofman added a comment.EditedTue, Nov 4, 22:20
  • Remove chronik-client peer dep by using outpoint instead of ScriptUtxo for the requiredUtxo param, consider renaming this param
  • Do not define and export unused consts from ecash-lib
  • Remove TODOs leftover from debugging
  • Clear explanation of the chainedTx shape, consider adding a spec in doc/ for ChainedXecTx or perhaps call it AirdropXec (vs AirdropToken which will require its own distinct spec)
modules/ecash-lib/package.json
50–56 ↗(On Diff #56417)

we can avoid this, see review comments below about substituting Outpoint

modules/ecash-lib/src/consts.ts
40–43 ↗(On Diff #56417)

unused, drop

modules/ecash-lib/src/payment/action.ts
53 ↗(On Diff #56417)

to avoid the chronik dependency, we could pass a distinct interface here, or simply Outpoint[]

...that would be the way to do it. Because ecash-wallet needs to find the exact utxo for it to work anyway. We do not need to pass the exact ScriptUtxo; we just need to pass its outIdx and txid so ecash-wallet can find the corresponding utxo in its available utxo set.

Harbormaster completed remote builds in B34947: Diff 56417.Tue, Nov 4, 22:24
bytesofman updated this revision to Diff 56418.Wed, Nov 5, 00:27

add a spec, clean up todos and logs, add more tests, use OutPoint instead of ScriptUtxo to specify requiredUtxo

bytesofman updated this revision to Diff 56419.Wed, Nov 5, 00:37

cleaner logic gate for handling utxo updates for unbroadcastable txs

Harbormaster completed remote builds in B34948: Diff 56418.Wed, Nov 5, 00:38
Harbormaster completed remote builds in B34949: Diff 56419.Wed, Nov 5, 00:56
bytesofman published this revision for review.Wed, Nov 5, 00:56
Fabien requested changes to this revision.Thu, Nov 6, 15:24
Fabien added a subscriber: Fabien.
Fabien added inline comments.
doc/standards/chained.md
1 ↗(On Diff #56419)

This should not be a specification but rather a design document (or code comment ?) that belong to ecash-lib/ecash-wallet

modules/ecash-wallet/src/wallet.ts
469 ↗(On Diff #56419)

Doc path to be updated

601 ↗(On Diff #56419)
961 ↗(On Diff #56419)

You should pass the max_tx_sersize as an action parameter with MAX_TX_SERSIZE as the default value instead, this makes your function much easier to test as you can set it to a lower value

4117 ↗(On Diff #56419)

Same, you should pass the max size instead of using MAX_TX_SERSIZE

This revision now requires changes to proceed.Thu, Nov 6, 15:24
bytesofman marked 5 inline comments as done.Thu, Nov 6, 19:21
bytesofman updated this revision to Diff 56501.Thu, Nov 6, 19:22

make maxTxSersize an optional param

bytesofman added inline comments.Thu, Nov 6, 19:29
modules/ecash-wallet/src/wallet.ts
4206 ↗(On Diff #56501)

currently not directly tested though it is an exported function

Not envisioned for this to be used outside of building a chained tx, so could be a private method, or could add some tests to make sure it's behaving as wanted

bytesofman added a comment.EditedThu, Nov 6, 19:29
  • would also split out the ecash-lib changes here into a separate diff with version bump and land that before this lands
Harbormaster completed remote builds in B35003: Diff 56501.Thu, Nov 6, 19:33
Fabien requested changes to this revision.Mon, Nov 10, 09:55
In D18867#430878, @bytesofman wrote:
  • would also split out the ecash-lib changes here into a separate diff with version bump and land that before this lands

Clearing my queue while this is pending

This revision now requires changes to proceed.Mon, Nov 10, 09:55
bytesofman mentioned this in D18918: [ecash-lib] Export new constants and interfaces to be used for chained txs in ecash-wallet.Mon, Nov 10, 17:32
bytesofman mentioned this in rABC43d39e207b44: [ecash-lib] Export new constants and interfaces to be used for chained txs in….Mon, Nov 10, 20:47
bytesofman updated this revision to Diff 56560.Mon, Nov 10, 20:57

rebase

Harbormaster completed remote builds in B35039: Diff 56560.Mon, Nov 10, 21:09
bytesofman added inline comments.Mon, Nov 10, 22:36
modules/ecash-wallet/src/wallet.ts
4206 ↗(On Diff #56501)

update: I looked at adding tests for this. It's possible, but I don't think it is +EV.

It's complicated to mock the BuiltTx and difficult for an observer/reviewer to tell if the tests are really adding value.

The functions being called in this method are tested individually, and this function is tested by integration tests that build chained txs. I think that's better than forcing a suite of direct tests for a function that is only used in the specific support of chained xec txs.

Fabien added inline comments.Tue, Nov 11, 10:00
modules/ecash-wallet/CHAINED.md
1 ↗(On Diff #56560)

No need for an uppercase file name here. I also doubt the frontmatter is useful but it's harmless, your call

modules/ecash-wallet/src/wallet.ts
467 ↗(On Diff #56560)

You might want to point to the readme as well

508 ↗(On Diff #56560)

what about clamping it as well, or error out if it's > MAX_TX_SERSIZE and < some min value ?

modules/ecash-wallet/test/transactions.test.ts
49 ↗(On Diff #56560)

that's verbose for sure

bytesofman marked 4 inline comments as done.Tue, Nov 11, 18:54
bytesofman added inline comments.
modules/ecash-wallet/src/wallet.ts
508 ↗(On Diff #56560)

if we do this, imo there's no point in having it as a param at all. mb there is some interesting edge case behavior here if a user specifies a negative value. but the reason for exposing the param is to allow power-user devs who presumably know what they are doing to create batched txs at some higher or lower sersize.

possible use cases

  • XEC changes this param at some point in the future
  • ecash-wallet adds support for other crypto projects that have a different MAX_TX_SERSIZE
  • power-user thinks 42,000 bytes is a good tx size and just wants to batch there for whatever reason

I think it's ok to let users specify a larger size. the tx will build, but an XEC node will not broadcast such a tx with a specific and legible error about the reason why.

Otherwise just keep it as a const and don't bother with the param.

modules/ecash-wallet/test/transactions.test.ts
49 ↗(On Diff #56560)

image.png (173×128 px, 41 KB)

bytesofman updated this revision to Diff 56563.Tue, Nov 11, 18:55
bytesofman marked 2 inline comments as done.

chained.md name and layout changes, code comment improvements and chained.md reference

Harbormaster completed remote builds in B35041: Diff 56563.Tue, Nov 11, 19:08
Fabien accepted this revision.Tue, Nov 11, 19:12
This revision is now accepted and ready to land.Tue, Nov 11, 19:12
Closed by commit rABCe60f96ad338e: [ecash-wallet, ecash-lib] Support chained XEC txs for XEC-only actions that… (authored by bytesofman). · Explain WhyTue, Nov 11, 19:22
This revision was automatically updated to reflect the committed changes.
bytesofman added a commit: rABCe60f96ad338e: [ecash-wallet, ecash-lib] Support chained XEC txs for XEC-only actions that….

Revision Contents
Changeset List

PathSize
modules/
ecash-wallet/
8 lines
61 lines
6 lines
2 lines
src/
275 lines
923 lines
test/
401 lines

Diff 56563

View Options

modules/ecash-wallet/README.md

Loading...
View Options

modules/ecash-wallet/chained.md

Loading...
View Options

modules/ecash-wallet/package-lock.json

Loading...
View Options

modules/ecash-wallet/package.json

Loading...
View Options

modules/ecash-wallet/src/wallet.test.ts

Loading...
View Options

modules/ecash-wallet/src/wallet.ts

Loading...
View Options

modules/ecash-wallet/test/transactions.test.ts

Loading...