Page MenuHomePhabricator

[ecash-lib] Implement ecashaddrjs functions in ecash-lib
Needs ReviewPublic

Authored by bytesofman on Sun, Dec 1, 00:47.

Details

Reviewers
emack
tobias_ruck
Group Reviewers
Restricted Project
Summary

ecashaddrjs has limped and served well despite numerous legacy dependencies and an older structure. Aside from some maintenance headaches, this was acceptable and "good enough."

However, we would like to

  • Add more address lib features
  • Optionally support more address types
  • Reduce the use of third-party dependencies

In starting to reduce the use of third-party dependencies, it was discovered that basic hash methods available in ecash-lib after initWasm were needed to match existing ecashaddrjs functionality without third-party dependencies. It would not make sense to have ecashaddrjs work only if initWasm() were called in ecash-lib -- so we move the address functions to ecash-lib.

The way js deps work, users who need only the address functions from ecash-lib will end up only installing those in the published version of their app. Users who want legacy ecashaddrjs features without dealing with initWasm may use the legacy ecashaddrjs, though it will be deprecated.

What this diff does

  • Match the functionality of ecashaddrjs but with no third-party dependencies (well, one dev-dependency for generating random hashes...we could lose it but it is useful to show we pass the same tests as the old lib)
  • Replace ecashaddrjs with now-local functions in ecash-lib
  • Update ecash-lib dockerfile to account for change
  • Update CI to account for everything that uses ecash-lib now needing b58-ts

Breaking changes when migrating from ecashaddrjs to ecash-lib
When I first started adding functionality to ecashaddrjs, I was hesitant to introduce any breaking changes. This led to a bunch of baggage. In this version,

  • We only support lowercase address types i.e. p2pkh or p2sh, as this is what ChronikClient supports
  • We store hash as a string. ecash-lib offers fromHex method to trivially convert this to a Uint8Array. But the string is also what ChronikClient expects and what most of my apps expect. Just picking one greatly enhances the effectiveness of typescript with these address methods. I considered storing both formats -- but that gets complicated as many helper functions also work with hash as a string.
  • We no longer validate against a small whitelist of accepted prefixes. We will encode any prefix.
  • We no longer decode prefixless addresses against a whitelist of accepted prefixes. We decode prefixless addresses against ecash prefix only.

Going forward, will replace ecashaddrjs with ecash-lib methods everywhere.

Test Plan

npm test

docker build -f ecash-lib.Dockerfile -t ecash-lib_local . builds

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

make constructor private, stop supporting so many input types, no overloaded constructor, better test organization

ok after leaving this for a day or two then looking at it again, understand the constructor overloading comment. cleaned it up, updated tests.

improve organization of address.test.ts

Fabien added inline comments.
modules/ecash-lib/src/address/address.ts
56 ↗(On Diff #51533)

What is the reason for storing both the address data and its representation ?

bytesofman added inline comments.
modules/ecash-lib/src/address/address.ts
56 ↗(On Diff #51533)

I think this is in reference to keeping both script and scriptHex in the class?

iterative decision from years of using the former lib and having the "we need string sometimes, but we need hex sometimes" always come up

That said -- it would be possible to store only one. However, the class may be created by one or the other. So sometimes we would throw one out when it might be needed again later.

I'm not totally sure how to square that optimization problem. If someone is using a loop to create 1000s of Address classes, then this is wasteful. However, the Address class isn't really meant for that. A dev looking for specific address methods should use the (more limited in scope) functions that are used within this class. These functions were exported by ecashaddrjs and are exported now in ecash-lib from cashaddr.ts.

In general, the use case of "just do one thing with an address" is always more speed optimized by using the specific functions in cashaddr and avoiding the use of this class.

For example, many possible Address use cases we do not need to store script or scriptHex at all (say converting a legacy address to a cash address) -- so both are arguably wasteful in this case. The Address class is engineered for user friendliness and one-off conversions.

Re: having both hex and uint8array. It's a weird quirk of crypto and JS development that scriptHex is useful in some dev contexts and script in others.

image.png (760×723 px, 137 KB)

I looked at also storing hash and hashHex ... arguably we would want this too. But when I got into it I thought I was adding too much complexity here. It's trivial enough to convert between the two types with toHex and fromHex also in this lib.

So "the line" of "is it worth doing this really?" is ... right between script and hash lol. In practice, I need to use script way more for app dev stuff (checking if sender is expected sender, checking if recipients include expected recipient, building txs).

This has been an uncomfortable thorn for a long time with ecashaddrjs, which (kinda) supports both, but returns conditionally based on the chronikReady param -- so typescript never knows which type you are getting from those functions bc it "could be either" even if you specify you want one or the other at the callsite. Keeping them in different keys with different return methods means typescript will now always know the type you are getting at the callsite.

modules/ecash-lib/src/address/address.ts
56 ↗(On Diff #51533)

Is was not about script but an address really is a prefix, a hash, a type (p2pkh/p2sh), and maybe a checksum. All the other data is representation and you can generate them on demand with methods like to_legacy_string(), to_cashaddr_string(prefix), to_script_hex() etc.

What I'm asking is why you feel the need to store the script at all, and the address as a string, and the encoding format. This is using much more memory and I don't really see the benefit.

modules/ecash-lib/src/address/address.ts
56 ↗(On Diff #51533)

Actually the prefix doesn't need to be stored if you use pass it down in the cashaddr rendering functions.

bytesofman added inline comments.
modules/ecash-lib/src/address/address.ts
56 ↗(On Diff #51533)

What I'm asking is why you feel the need to store the script at all,

image.png (470×531 px, 159 KB)

good point, removed script and scriptHex

and the address as a string, and the encoding format. This is using much more memory and I don't really see the benefit.

imo there is still value in storing the address as a string and in storing its encoding. Unless the user is asking for a new prefix or a conversion, it makes sense that Address preserves the address of its construction.

In this way, we preserve for example qz2708636snqhsxu8wnlka78h6fdp77ar59jrf5035 as distinct from ecash:qz2708636snqhsxu8wnlka78h6fdp77ar59jrf5035, or, similarly, any other arbitrary prefix the user may have selected. We would have no way of determining this in a method.

Similar with a legacy address -- the user presumably has some reason for wanting their address in legacy format, so the Address object should preserve this (unless the user uses Address to construct a new Address of a different type, in which case there is now reason to believe it should be that new type). As we extend to support other address types, this would become more important.

stop storing script and scriptHex as these can always be returned accurately by method

contrib/teamcity/build-configurations.yml
82 ↗(On Diff #51553)

At some point this is getting madness. We crossed that point long ago already.
Please fix this and find a way to make this maintainable.

tobias_ruck added inline comments.
modules/ecash-lib/src/address/address.ts
92 ↗(On Diff #51553)

Since you export this (?) it should have a good name

107 ↗(On Diff #51553)

I still really don’t understand why we have to have an uber constructor, just to make it private 🤔

Instead, it should be a (private) constructor that just assigns the params to this (so like 5 assignments), and then all the logic in the branches should be moved to the corresponding static function (eg Address.p2sh).

Much more decentralized, much more maintainable, wow

215 ↗(On Diff #51553)

Oversight?

220 ↗(On Diff #51553)

I think it’s unexpected that this is not a pure function. You should create a new Address obj.
Haskell would be proud

242 ↗(On Diff #51553)

Same here—I don’t think it’s correct to modify this here

252 ↗(On Diff #51553)

Consider throwing an error in this case—it seems like the user is using it incorrectly and should be informed.

260 ↗(On Diff #51553)

Same here

This revision now requires changes to proceed.Fri, Dec 13, 10:26
bytesofman added inline comments.
contrib/teamcity/build-configurations.yml
82 ↗(On Diff #51553)

working on this.

the benefit of deprecating ecashaddrjs -- and thus removing this double dependency from most of these (crowded) steps -- imo justifies landing this diff before refactoring the CI dep management. The overall need to simplify CI is a valid problem but outside the scope of this diff.

Since this diff will allow us to simplify the existing setup, I do not think this should be a blocker.

modules/ecash-lib/src/address/address.ts
92 ↗(On Diff #51553)

moot after cleaning up the constructor

107 ↗(On Diff #51553)

yes this is much better, thanks

root cause here

  • started with complex overengineered constructor based on personally derived use habits for addr conversion
  • dramatically simplified from this...difficult to see what was left was still way more complicated than it had to be
bytesofman marked 3 inline comments as done.

simplify constructor, pure functions for legacy() and cash(), better error handling for withPrefix

Tail of the build log:

   Compiling ripemd v0.1.3
   Compiling sha2 v0.10.8
   Compiling wasm-bindgen-backend v0.2.92
   Compiling ecash-secp256k1 v0.30.0 (/work/modules/ecash-secp256k1)
   Compiling thiserror-impl v2.0.4
   Compiling wasm-bindgen-macro-support v0.2.92
   Compiling wasm-bindgen-macro v0.2.92
   Compiling ecash-lib-wasm v0.1.0 (/work/modules/ecash-lib-wasm)
    Finished release-wasm [optimized] target(s) in 8.03s
/work/modules/b58-ts /work/modules/ecash-lib-wasm /work/modules/ecash-script /work/modules/chronik-client /work/modules/mock-chronik-client /work/modules/ecashaddrjs /work/abc-ci-builds/cashtab-tests
npm warn deprecated inflight@1.0.6: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
npm warn deprecated @humanwhocodes/config-array@0.13.0: Use @eslint/config-array instead
npm warn deprecated rimraf@3.0.2: Rimraf versions prior to v4 are no longer supported
npm warn deprecated glob@7.2.0: Glob versions prior to v9 are no longer supported
npm warn deprecated @humanwhocodes/object-schema@2.0.3: Use @eslint/object-schema instead
npm warn deprecated glob@8.1.0: Glob versions prior to v9 are no longer supported
npm warn deprecated eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options.

> b58-ts@0.1.0 prepublish
> npm run build


> b58-ts@0.1.0 build
> tsc && tsc -p ./tsconfig.build.json


added 213 packages, and audited 215 packages in 4s

49 packages are looking for funding
  run `npm fund` for details

2 low severity vulnerabilities

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.

> b58-ts@0.1.0 build
> tsc && tsc -p ./tsconfig.build.json

/work/modules/ecash-lib /work/modules/b58-ts /work/modules/ecash-lib-wasm /work/modules/ecash-script /work/modules/chronik-client /work/modules/mock-chronik-client /work/modules/ecashaddrjs /work/abc-ci-builds/cashtab-tests

added 364 packages, and audited 368 packages in 2s

60 packages are looking for funding
  run `npm fund` for details

2 vulnerabilities (1 moderate, 1 high)

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

> ecash-lib@1.1.0 build
> tsc && tsc -p ./tsconfig.build.json && cp -r ./src/ffi ./dist

src/address/address.test.ts(19,5): error TS2614: Module '"./address.js"' has no exported member 'SpecifiedConstructorParam'. Did you mean to use 'import SpecifiedConstructorParam from "./address.js"' instead?
Build cashtab-tests failed with exit code 2

Tail of the build log:

   Compiling digest v0.10.7
   Compiling sha2 v0.10.8
   Compiling ripemd v0.1.3
   Compiling wasm-bindgen-backend v0.2.92
   Compiling ecash-secp256k1 v0.30.0 (/work/modules/ecash-secp256k1)
   Compiling thiserror-impl v2.0.4
   Compiling wasm-bindgen-macro-support v0.2.92
   Compiling wasm-bindgen-macro v0.2.92
   Compiling ecash-lib-wasm v0.1.0 (/work/modules/ecash-lib-wasm)
    Finished release-wasm [optimized] target(s) in 7.41s
Test depends on chronik-client. Building TypeScript...
/work/modules/chronik-client /work/modules/ecash-lib-wasm /work/modules/ecashaddrjs /work/modules/b58-ts /work/abc-ci-builds/ecash-lib-tests

> chronik-client@2.1.0 prepublish
> npm run build


> chronik-client@2.1.0 build
> tsc


added 265 packages, and audited 267 packages in 5s

48 packages are looking for funding
  run `npm fund` for details

5 vulnerabilities (2 low, 1 moderate, 2 high)

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.
Test does not depend on mock-chronik-client, skipping mock-chronik-client dependencies...
Test does not depend on ecash-lib
Test does not depend on ecash-agora
Test does not depend on ecash-script, skipping ecash-script dependencies...
Test does not depend on ecash-coinselect, skipping ecash-coinselect dependencies...
/work/modules/ecash-lib /work/modules/chronik-client /work/modules/ecash-lib-wasm /work/modules/ecashaddrjs /work/modules/b58-ts /work/abc-ci-builds/ecash-lib-tests

added 364 packages, and audited 368 packages in 2s

60 packages are looking for funding
  run `npm fund` for details

2 vulnerabilities (1 moderate, 1 high)

To address all issues, run:
  npm audit fix

Run `npm audit` for details.
CI configured to test build. Building...

> ecash-lib@1.1.0 build
> tsc && tsc -p ./tsconfig.build.json && cp -r ./src/ffi ./dist

src/address/address.test.ts(19,5): error TS2614: Module '"./address.js"' has no exported member 'SpecifiedConstructorParam'. Did you mean to use 'import SpecifiedConstructorParam from "./address.js"' instead?
Build ecash-lib-tests failed with exit code 2

Tail of the build log:

   Compiling syn v2.0.90
   Compiling ecash-secp256k1-sys v0.10.0 (/work/modules/ecash-secp256k1/ecash-secp256k1-sys)
   Compiling block-buffer v0.10.4
   Compiling crypto-common v0.1.6
   Compiling digest v0.10.7
   Compiling sha2 v0.10.8
   Compiling ripemd v0.1.3
   Compiling wasm-bindgen-backend v0.2.92
   Compiling ecash-secp256k1 v0.30.0 (/work/modules/ecash-secp256k1)
   Compiling thiserror-impl v2.0.4
   Compiling wasm-bindgen-macro-support v0.2.92
   Compiling wasm-bindgen-macro v0.2.92
   Compiling ecash-lib-wasm v0.1.0 (/work/modules/ecash-lib-wasm)
    Finished release-wasm [optimized] target(s) in 7.95s
Test depends on chronik-client. Building TypeScript...
/work/modules/chronik-client /work/modules/ecash-lib-wasm /work/modules/ecashaddrjs /work/modules/b58-ts /work/abc-ci-builds/ecash-agora-tests

> chronik-client@2.1.0 prepublish
> npm run build


> chronik-client@2.1.0 build
> tsc


added 265 packages, and audited 267 packages in 5s

48 packages are looking for funding
  run `npm fund` for details

5 vulnerabilities (2 low, 1 moderate, 2 high)

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.
Test does not depend on mock-chronik-client, skipping mock-chronik-client dependencies...
Test depends on ecash-lib. Building TypeScript...
/work/modules/ecash-lib /work/modules/chronik-client /work/modules/ecash-lib-wasm /work/modules/ecashaddrjs /work/modules/b58-ts /work/abc-ci-builds/ecash-agora-tests

added 364 packages, and audited 368 packages in 2s

60 packages are looking for funding
  run `npm fund` for details

2 vulnerabilities (1 moderate, 1 high)

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

> ecash-lib@1.1.0 build
> tsc && tsc -p ./tsconfig.build.json && cp -r ./src/ffi ./dist

src/address/address.test.ts(19,5): error TS2614: Module '"./address.js"' has no exported member 'SpecifiedConstructorParam'. Did you mean to use 'import SpecifiedConstructorParam from "./address.js"' instead?
Build ecash-agora-tests failed with exit code 2

Tail of the build log:

   Compiling quote v1.0.37
   Compiling syn v2.0.90
   Compiling ecash-secp256k1-sys v0.10.0 (/work/modules/ecash-secp256k1/ecash-secp256k1-sys)
   Compiling block-buffer v0.10.4
   Compiling crypto-common v0.1.6
   Compiling digest v0.10.7
   Compiling ripemd v0.1.3
   Compiling sha2 v0.10.8
   Compiling wasm-bindgen-backend v0.2.92
   Compiling ecash-secp256k1 v0.30.0 (/work/modules/ecash-secp256k1)
   Compiling thiserror-impl v2.0.4
   Compiling wasm-bindgen-macro-support v0.2.92
   Compiling wasm-bindgen-macro v0.2.92
   Compiling ecash-lib-wasm v0.1.0 (/work/modules/ecash-lib-wasm)
    Finished release-wasm [optimized] target(s) in 4.73s
Test depends on chronik-client. Building TypeScript...
/work/modules/chronik-client /work/modules/ecash-lib-wasm /work/modules/ecashaddrjs /work/modules/b58-ts /work/abc-ci-builds/ecash-agora-integration-tests

> chronik-client@2.1.0 prepublish
> npm run build


> chronik-client@2.1.0 build
> tsc


added 265 packages, and audited 267 packages in 5s

48 packages are looking for funding
  run `npm fund` for details

5 vulnerabilities (2 low, 1 moderate, 2 high)

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.
Test depends on ecash-lib. Building TypeScript...
/work/modules/ecash-lib /work/modules/chronik-client /work/modules/ecash-lib-wasm /work/modules/ecashaddrjs /work/modules/b58-ts /work/abc-ci-builds/ecash-agora-integration-tests

added 364 packages, and audited 368 packages in 2s

60 packages are looking for funding
  run `npm fund` for details

2 vulnerabilities (1 moderate, 1 high)

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

> ecash-lib@1.1.0 build
> tsc && tsc -p ./tsconfig.build.json && cp -r ./src/ffi ./dist

src/address/address.test.ts(19,5): error TS2614: Module '"./address.js"' has no exported member 'SpecifiedConstructorParam'. Did you mean to use 'import SpecifiedConstructorParam from "./address.js"' instead?
Build ecash-agora-integration-tests failed with exit code 2
tobias_ruck added inline comments.
modules/ecash-lib/src/address/address.ts
47 ↗(On Diff #51605)

Let's factor this out into it's own type: export type AddressEncoding = 'cashaddr' | 'legacy';

92 ↗(On Diff #51605)
108 ↗(On Diff #51605)

the thing we get here is not a constructor (but its params)

115 ↗(On Diff #51605)

are you sure this isn't redundant?

122 ↗(On Diff #51605)

This isn't too helpful anymore tbh.

Probably best to add actual /** docs */ here for each static func

123–139 ↗(On Diff #51605)

thoughts on simplifying like this? (and similarly for the others)

139–140 ↗(On Diff #51605)

personal style preference—functions are usually separated by a blank line, but up to you

157 ↗(On Diff #51605)

thoughts on a parse static function that can handle either cashddr or legacy?

217–224 ↗(On Diff #51605)
226 ↗(On Diff #51605)

similar here, inline the prefix and make it a single expr (mostly for style points ofc)

249 ↗(On Diff #51605)

these I think should also be moved into the new Address(...) expression

This revision now requires changes to proceed.Sun, Dec 15, 23:38
bytesofman marked 10 inline comments as done.

implementing feedback (add parse method, clean up existing methods, doc, remove unused interfaces and imports now that stuff is cleaned up)

bytesofman added inline comments.
modules/ecash-lib/src/address/address.ts
115 ↗(On Diff #51605)

...I swear TS made me do it before. But it's ok with it now.

tobias_ruck added inline comments.
modules/ecash-lib/src/address/address.ts
21–22

not too helpful IMO for new devs, maybe something like

"The hash this address encodes. It's part of the Script this address represents."

27

this is not correct anymore I think

63–64
69–70
83–84

(my personal style, feel free to ignore, I like to keep newlines low in comments for readability)

143–150

my personal style

161–167
171
181–182
184–191
201

don't duplicate the code, use either fromScript to call fromScriptHex or vice versa

This revision now requires changes to proceed.Mon, Dec 16, 12:27

adding docs, style clean-ups, remove default type as it is not used anymore, remove unnecessary code duplication

tobias_ruck added inline comments.
modules/ecash-lib/src/address/cashaddr.ts
486 ↗(On Diff #51619)

consider moving this to legacyaddr.ts; ideally we can disentangle legacy from cashaddr in the future (so that they don't depend on each other)

486 ↗(On Diff #51619)

you might also consider moving this code into the Address class, and then use the class itself (instead of Address using this function) for this function (if we even need toLegacyAddress at all):
Address.fromCashAddress(cashaddress).legacy().toString()

489–491 ↗(On Diff #51619)
504 ↗(On Diff #51619)

here you're duplicating the version bytes

551–552 ↗(On Diff #51619)

this should be moved to legacyaddr.ts and maybe have a legacy prefix
You should find also a data structure that works for the required data structures so we only have to define this once (ideally also something that lends itself to support other blockchains)

607 ↗(On Diff #51619)

same here—better move this to legacyaddr.ts

This revision now requires changes to proceed.Mon, Dec 16, 14:22
bytesofman added inline comments.
modules/ecash-lib/src/address/cashaddr.ts
486 ↗(On Diff #51619)

conversion to legacy address is something we can only do with a cashaddr, seems to belong here

486 ↗(On Diff #51619)

in this case would just get rid of this function

i think there is value in keeping these functions separate, if for no other reason it simplifies specific unit testing

489–491 ↗(On Diff #51619)

refactored this to change behavior a bit (throw if user tries to convert legacy to legacy)

551–552 ↗(On Diff #51619)

imo easy enough to change the data structure to support other chains once we have another chain to support and know what the shape should be. typescript and the unit tests will make any update straightforward.

not totally sure what it should be otherwise (tho i imagine you do since you have made the diff that does this already)

bytesofman marked 4 inline comments as done.

change toLegacyAddress behavior to throw if called with legacy address, move legacy functions to legacyaddr.ts, define toLegacyAddress in address.ts, organize and update tests accordingly