Page MenuHomePhabricator

[ecash-lib] Support address key in target outputs
ClosedPublic

Authored by bytesofman on Jul 5 2024, 22:23.

Details

Summary

Explored preparing all targetOutputs with Script in Cashtab (see D16427).

While this is doable, it is a weird API to standardize. Most developers will be working with address. For example, scanning QR codes gets an address. User copy paste entry is expected to be an address (or maybe an alias).

Expecting developers to always convert to ecash-lib Script type before calling the library is unreasonable. ecash-lib should just handle address.

Test Plan

npm test, CI

Diff Detail

Repository
rABC Bitcoin ABC
Branch
ecash-lib-support-address
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 29493
Build 58522: Build Diffecash-lib-integration-tests · ecash-agora-integration-tests · ecash-lib-tests
Build 58521: arc lint + arc unit

Event Timeline

throw error for ambiguous output, bump version, update changelog, add test

return hash from ecashaddrjs as uint8array instead of converting to string then converting back to uint8array

doxygen for new type TxOutputAddress

bytesofman published this revision for review.Jul 5 2024, 23:05
bytesofman added inline comments.
modules/ecash-lib/package.json
48

We do not need to change ecash-lib.Dockerfile or teamcity CI as ecashaddrjs is already built before npm ci on ecash-lib, as it is a dependency of chronik-client --- an existing dev dependency of this library.

modules/ecash-lib/src/txBuilder.ts
45

We define a new interface here that is only used to input into the TxBuilder, as we do not want to extend any of the other TxOutput methods to this type (copyTxOutput, writeTxOutput, readTxOutput)

The only thing we want to do with TxOutputAddress is convert it to TxOutput

123

mb this is overkill but I can see this causing problems. behavior without this check would be to always ignore the script key in a given output with an address key

potentially quite difficult to debug

I have several thoughts by looking at this code:

  • Should we add a Cashaddr type to avoid dealing with raw strings ?
  • Wouldn't be cleaner to TxOutput be like:
export interface TxOutput {
    /** Value in satoshis of the output (1 XEC = 100 satoshis) */
    value: number | bigint;
    /** Destination of the output */
    dest: Script | Cashaddr;
}
  • Should we add a Cashaddr type to avoid dealing with raw strings ?

Open to this but not sure what it would be like in practice. For app dev, we will usually come across addresses as strings from user input. Straightforward enough to validate these at input -- standard practice, happens in Cashtab -- but there isn't any way to "enforce" this validation, i.e. make sure that anyone calling TxBuilder only does so with valid CashAddr types at address key. We need to validate address input in ecash-lib no matter what. So, if we require some kind of type-checked valid address string, we might as well just require the user to convert the string address into a Script.

  • Wouldn't be cleaner to TxOutput be like:
export interface TxOutput {
    /** Value in satoshis of the output (1 XEC = 100 satoshis) */
    value: number | bigint;
    /** Destination of the output */
    dest: Script | Cashaddr;
}

Potentially. However, Script is already optimized for ecash-lib language, i.e. it includes Uint8Array at bytecode which can be written/read/manipulated just like all the other non-address Script possibilities of ecash. We don't want to ever do with with addresses ... they just need to be translated to Script so that ecash-lib can get them into a raw tx. If we have this sort of combined type, then we always need to check it to make sure "if dest is a Cashaddr, convert it to a script".

The way it's set up now -- TxOutputAddress is only ever used when calling TxBuilder. TxBuilder converts it to TxOutput before doing anything else with it. So, everything under the hood is always TxOutput and Script. ecash-lib only works with TxOutput; users may call it with TxOutputAddress.

High level issue here:

"an address (itself a complicated string with different formats and rules) is actually a special encoding of a Script, and you need to know if it is p2pkh or p2sh (what) and make sure you convert it properly"

--> this is esoteric knowledge for many app devs.

If we think the dev should always handle this at the app level, then it's appropriate to accept only Script input. imo this is too much of a hurdle rate for app dev. Should be able to build an app that sends and receives txs without being intimately familiar with how txs are built and signed.

OK I get a better sense of what API you're trying to build then. I think creating the new type is the wrong approach because it makes the API inconsistent from the callsite perspective. You can specify any output as an TxBuilderAddress but the change output has to be a Script ? It is simpler imo to require the callsite to use Script.fromAddress() every time it uses an address: it's explicit and consistent because you will do that for the change output as well. It avoids the issue "address vs script as a key but not both" also which makes the code more robust by design.

What do you think ?

do not support new TxOutputAddress type, instead require Script.fromAddress() at callsite

OK I get a better sense of what API you're trying to build then. I think creating the new type is the wrong approach because it makes the API inconsistent from the callsite perspective. You can specify any output as an TxBuilderAddress but the change output has to be a Script ? It is simpler imo to require the callsite to use Script.fromAddress() every time it uses an address: it's explicit and consistent because you will do that for the change output as well. It avoids the issue "address vs script as a key but not both" also which makes the code more robust by design.

What do you think ?

Good point, this is a better approach.

The main thing is we don't want devs having to write their own address to Script method for every app that uses ecash-lib. Having the method available in ecash-lib solves this problem.

Fabien requested changes to this revision.Jul 9 2024, 09:06

Can you add a test case that uses Script.fromAddress for the change output as well ?
Otherwise the code LGTM

This revision now requires changes to proceed.Jul 9 2024, 09:06

add test case for Script.fromAddress use as leftover (change) output

tobias_ruck added a subscriber: tobias_ruck.
tobias_ruck added inline comments.
modules/ecash-lib/README.md
102 ↗(On Diff #48548)

I think this is more correct

bytesofman marked an inline comment as done.

README patch per recommendation

This revision is now accepted and ready to land.Jul 9 2024, 12:06