Misc→fee and cleanup→slush were leaving XEC change on seller (postage pulled in for the miner fee). That leftover is itself odd XEC, so scheduled maintain swept it again every 5 minutes. Add optional changeScript on payment.Action so seller sweeps send change to slush. Also log each maintain broadcast txid (and the summary line) so those txs are inspectable. Bump ecash-lib 4.14.0, ecash-wallet 6.2.0, and ecash-agora 4.2.8.
Details
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABC22dfb0a6f650: [alp-dex] Send seller-sweep leftover to slush
- cd apps/alp-dex && npm test
- cd modules/ecash-wallet && npm test -- --grep changeScript
- Confirm a seller misc sweep: fee output + leftover on slush, no new odd XEC on seller
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (ai-review) passed.
CodeRabbit Review
Diff : committed changes only
Compare : HEAD → master
Directory : work
────────────────────────────────────────
(\(\
(• .•) Your documentation is 'TBD'. My patience is 'TBF' (to be found).
────────────────────────────────────────
Review complete
No findings ✔
7 files reviewed:
- apps/alp-dex/index.ts
- apps/alp-dex/src/inventory/actions.ts
- apps/alp-dex/src/inventory/maintain.ts
- apps/alp-dex/test/inventory.test.ts
- modules/ecash-lib/src/payment/action.ts
- modules/ecash-wallet/src/wallet.test.ts
- modules/ecash-wallet/src/wallet.ts
────────────────────────────────────────
| modules/ecash-lib/src/payment/action.ts | ||
|---|---|---|
| 79 ↗ | (On Diff #61158) | There's an argument to be made for keeping this change out of ecash-lib and ecash-wallet. It is quite a niche use case for "single address wallet wants to send change to some other arbitrary address." That said, the use case is real here, and could be real in other places. The optional param doesn't hurt existing features. The alternative, if we want this functionality, is to manually calc fees and construct the tx in alp-dex ... which imo is worse than this potentially lightly used wallet extension. The arena of "single address wallets shape their UTXO sets in automated ways in response to inventory management or other server goals" is relatively new. In practice though it seems to be useful for AMMs, which could be expected to grow and have other related use cases. |
Comment Actions
the ecash-lib and ecash-wallet wallet versions need to be bumped
| apps/alp-dex/test/inventory.test.ts | ||
|---|---|---|
| 520 ↗ | (On Diff #61158) | not sure it's worth testing a log function tbh, it's mostly copy pasta and will break quickly |
| modules/ecash-lib/src/payment/action.ts | ||
| 79 ↗ | (On Diff #61158) | I'm fine with this, it doesn't break the API so no problem. But what is wrong is the name: it's called change everywhere but here for some reason it's called leftover; please keep it consistent |
Comment Actions
Rename leftoverScript to changeScript, drop the log-formatter unit test, and bump ecash-lib 4.14.0, ecash-wallet 6.2.0, and ecash-agora 4.2.8.
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (ai-review) passed.
CodeRabbit Review
Diff : committed changes only
Compare : HEAD → master
Directory : work
────────────────────────────────────────
(\(\
(• .•) There is nothing quite so useless as doing with great efficiency something that should not be done at all. - Peter Drucker
────────────────────────────────────────────────────────────────────────
minor [Maintainability & Code Quality] → ]8;;vscode://file//work/modules/ecash-lib/src/payment/action.ts:76modules/ecash-lib/src/payment/action.ts:76]8;; Correct the noChange contract. Remove “Ignored when noChange is true.” An action that contains both properties throws changeScript cannot be combined with noChange, as covered by the wallet test. The current text describes two incompatible behaviors. Proposed fix - * Ignored when `noChange` is true. Cannot be combined with `noChange`. + * Cannot be combined with `noChange`.
────────────────────────────────────────────────────────────────────────
major [Data Integrity & Integration] → ]8;;vscode://file//work/modules/ecash-wallet/src/wallet.ts:2190modules/ecash-wallet/src/wallet.ts:2190]8;; Account for custom change-script sizes in chained fee planning. _buildSizeLimitExceededChained calculates output capacity and fees using P2PKH-sized placeholders, but this line accepts any Script. A larger action.changeScript can make the Alpha fee estimate too small, causing signing failure or a transaction above maxTxSersize. Size the chain with the actual script or reject unsupported script types before this path.
────────────────────────────────────────────────────────────────────────
major [Data Integrity & Integration] → ]8;;vscode://file//work/modules/ecash-wallet/src/wallet.ts:3423modules/ecash-wallet/src/wallet.ts:3423]8;; Propagate changeScript through chained builds. This supports changeScript only in the normal build path. Chained token sends still use this._wallet.getChangeScript() at Line [1854], and _buildMultiTokenSend drops the field when creating subAction at Lines [1419-1427]. An action that enters TOKEN_SEND_EXCEEDS_MAX_OUTPUTS can therefore send change to the wrong address. Preserve changeScript in every derived action and chained change-output replacement.
────────────────────────────────────────
Review complete
3 findings ✔
Major 2
Minor 1
13 files reviewed:
- apps/alp-dex/index.ts
- apps/alp-dex/src/inventory/actions.ts
- apps/alp-dex/src/inventory/maintain.ts
- apps/alp-dex/test/inventory.test.ts
- modules/ecash-agora/README.md
- modules/ecash-agora/package.json
- modules/ecash-lib/README.md
- modules/ecash-lib/package.json
- modules/ecash-lib/src/payment/action.ts
- modules/ecash-wallet/README.md
... and 3 more files
────────────────────────────────────────
Print all AI prompts: coderabbit review --show-prompts