This avoids pulling the ecashaddrjs lib or the ecash-lib when using the wallet for simple transactions.
Details
Details
- Reviewers
bytesofman - Group Reviewers
Restricted Project - Commits
- rABC53741ac9cfbf: [ecash-wallet] Accept addresses in the transaction ouputs
npm test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| modules/ecash-lib/src/payment/output.ts | ||
|---|---|---|
| 16 ↗ | (On Diff #54944) | maybe something like this? https://grok.com/share/c2hhcmQtMg%3D%3D_bf48f5c3-d4ec-4e01-8f2d-72accda9acbb though, without implementing and seeing what kind of ts issues this propagates, it's hard to say if this would be an improvement. simplest would be to just validate in ecash-wallet. Either way, we should at least comment that the expected use is either/or |
| modules/ecash-wallet/src/wallet.test.ts | ||
| 1765–1769 ↗ | (On Diff #54944) | imo throwing an error here would be better behavior there's no good reason for a user to provide both |
| modules/ecash-wallet/src/wallet.ts | ||
| 1413 ↗ | (On Diff #54944) | in this way, we can have outputs with both address and script keys that do not correspond to each other I think it would be cleaner if we validate for either/or -- either address or script may be specified, but not both. |
| modules/ecash-lib/src/payment/output.ts | ||
|---|---|---|
| 16 ↗ | (On Diff #54944) | I considered that but it's not the interface I want because I might want to build the action once and reuse it in my callsite (repeatedly sending txs to the same recipient with the same amount). Using an union would in turn make the code much more convoluted because I could no longer simply fill up the script field and leave the logic identical. In any case you're correct that this deserves some more comment to explain that the address takes precedence. |
| modules/ecash-wallet/src/wallet.test.ts | ||
| 1765–1769 ↗ | (On Diff #54944) | Same rationale, this is about reusing the testAction object. I'm open to discussion on this one, if this is something we really want to avoid then I can change it to error out instead (this was in fact my first design which I changed for this solution). |
| modules/ecash-wallet/src/wallet.ts | ||
| 1413 ↗ | (On Diff #54944) | Again, same rationale. One thing that could be done and sits in the middle is to check if the script exists and differs from the decoded address, so if the address did change and forbid that behavior. I dont' think this adds much value as a feature tho, let me know what you think |
Comment Actions
Use the XOR type approach that prevents the user from having both address and script set in the action.
This requires some update to the finalizeOutputs as well so it returns the massaged outputs instead of
overwriting the input action, which is now immutable (not annotated as such in this diff).
Comment Actions
yes this is a much better approach.
the whole "finalizeOutputs" does not return anything thing is an artifact of extensive iterative dev on validating inputs, it's not the best API
some small change requests
| modules/ecash-wallet/src/wallet.ts | ||
|---|---|---|
| 1362 ↗ | (On Diff #54949) | also: paymentOutputsToTxOutputs it is a pretty simple function, but we should include tests for it as it is doing an easily-tested thing where getting it wrong would be very bad |
| modules/ecash-wallet/src/wallet.ts | ||
|---|---|---|
| 1362 ↗ | (On Diff #54949) | it's somehow covered by the finalizeOutputs tests (as it was before this diff) but it can't hurt to add some dedicated tests as well |
Comment Actions
Rename and add unit tests for paymentOutputsToTxOutputs. The drawback is that it is now exported but that's no big deal