Page MenuHomePhabricator

[ecash-lib] Improve address handling of ecash dev libraries
ClosedPublic

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

Details

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. Then, though, we still would like to not require the entire ecash-lib dependency for apps like chronik-client which barely use ecashaddrjs functionality.

Solution

  • Remove all dependencies from ecashaddrjs; this requires getting rid of toLegacy, which we add into ecash-lib
  • Breaking change version bump for ecashaddrjs to get rid of other technical debt (tree-shakeable exports so apps only pull in the functions they need, not the whole thing, and better typing, no more chronikReady param which confused typescript about return types)
  • New Address class in ecash-lib with full-featured address management
  • New legacyaddr.ts functions in ecash-lib for handling legacy addresses with zero deps (since we have hashing available in ecash-lib, and now also b58-ts with no deps)
  • Implement ecashaddrjs with breaking change in all monorepo apps
Test Plan

npm test

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

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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 ↗(On Diff #51607)

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 ↗(On Diff #51607)

this is not correct anymore I think

63–64 ↗(On Diff #51607)
69–70 ↗(On Diff #51607)
83–84 ↗(On Diff #51607)

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

143–150 ↗(On Diff #51607)

my personal style

161–167 ↗(On Diff #51607)
171 ↗(On Diff #51607)
181–182 ↗(On Diff #51607)
184–191 ↗(On Diff #51607)
201 ↗(On Diff #51607)

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

change toLegacyAddress behavior to throw if called with legacy address

What is the benefit vs a no-op ? Imagine I'm a mining pool and I want to convert all my miner addresses to legacy (real world use case) even if they are using BCH or XEC addresses. I would like to just use to_legacy() and not have to first check it's not legacy format already

What is the benefit vs a no-op ? Imagine I'm a mining pool and I want to convert all my miner addresses to legacy (real world use case) even if they are using BCH or XEC addresses. I would like to just use to_legacy() and not have to first check it's not legacy format already

Happy to revert it again, good rationale

pull out cashaddr logic from ecash-lib, remove dependencies from ecashaddrjs, major version bump for ecashaddrjs, implement ecashaddrjs 2.0.0 in all monorepo libraries, modify toLegacyAddress so it is a no-op if called with legacy addr

bytesofman retitled this revision from [ecash-lib] Implement ecashaddrjs functions in ecash-lib to [ecash-lib] Improve address handling of ecash dev libraries.Thu, Dec 19, 07:42
tobias_ruck added inline comments.
modules/ecash-lib/README.md
108 ↗(On Diff #51673)
modules/ecash-lib/tests/txBuilder.test.ts
8 ↗(On Diff #51673)

usually external and own dependencies are grouped, but you can also remove the lines separating it

modules/ecashaddrjs/README.md
167 ↗(On Diff #51673)

I think you mean "remove"? Deprecate afaik means that a behavior is still supported but marked that it shouldn't be used anymore. Probably better to use "Remove" instead for this

This revision is now accepted and ready to land.Thu, Dec 19, 23:24
bytesofman marked 3 inline comments as done.

rebase, README improvements, preserve txBuilder.test.ts dependency groupings