Details

Reviewers

Group Reviewers

Restricted Project

Summary

ecash-lib methods that require initWasm() will not work if ecash-lib is a dep of a lib. For example, if we are trying to use ecash-agora in an app, and we need to use ecash-agora methods that require initWasm, like acceptTx -- this will fail even if we initWasm() before we call it.

It will pass though if we initWasm in ecash-agora before ecash-agora calls a function like shaRmd160 (which needs initWasm to work.

In general it is best practice not to modify globalThis. However in getting webassembly to work as a deb of a dep, I do not have a better idea (also not sure this will work tbh).

Test Plan

CI to confirm this does not break existing monorepo apps.

How I tested this

I published this as an rc
I installed it as a dep of a lib (a draft wallet class I am working on)
Tests that used this lib were failing before installing this rc, they passed after
I installed this rc as a dep of ecash-agora and published that as an rc
Tests that were failing because initWasm() was not making methods available passed after replacing currently-published ecash-agora with that version

So, ecash-agora@0.2.1-rc4 and ecash-lib@1.2.2-rc4 solve the issue

Diff Detail

Repository

rABC Bitcoin ABC

Branch

ecash-lib-alt-initwasm

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 31918
Build 63328: Build Diff	ecash-lib-integration-tests · ecash-agora-integration-tests · cashtab-tests · ecash-lib-tests · ecash-agora-tests
Build 63327: arc lint + arc unit

Event Timeline

bytesofman created this revision.Sat, Jan 4, 00:04

Herald added a reviewer: Restricted Project. · View Herald TranscriptSat, Jan 4, 00:04

Harbormaster completed remote builds in B31908: Diff 51950.Sat, Jan 4, 00:14

modify for non-local deps as will need to iterate through rcs to test published version

ok in testing ... seems to be working for hashes but needs some work on ecc

console.log(globalThis.ecashlib)

{
  HASHES: {
    sha256: [Function (anonymous)],
    sha256d: [Function (anonymous)],
    shaRmd160: [Function (anonymous)]
  }
}

and still get the TypeError: globalThis.ecashlib.Ecc is not a constructor which makes sense since clearly it has not been properly added to globalThis

Harbormaster completed remote builds in B31909: Diff 51951.Sat, Jan 4, 00:33

do not overwrite the object, more ts shims

Harbormaster completed remote builds in B31910: Diff 51953.Sat, Jan 4, 00:52

return an alt dummy "Unimplemented" ecc if unimplemented, allow calling with new Ecc()

Harbormaster completed remote builds in B31911: Diff 51954.Sat, Jan 4, 01:36

do not import the merged type as it causes issues with rel dep apps

Harbormaster completed remote builds in B31912: Diff 51955.Sat, Jan 4, 01:50

clean up comments, remove rc version

Harbormaster completed remote builds in B31913: Diff 51956.Sat, Jan 4, 05:01

Build Bitcoin ABC Diffs / Diff Testing (ecash-agora-tests) failed.

Tail of the build log:

Build 'Bitcoin ABC Diffs / Diff Testing' #N/A, branch 'refs/tags/phabricator/diff/51956'
Triggered 2025-01-04 05:01:30 by 'Phabricator Staging (phabricator-staging)'
Started 2025-01-04 05:01:37 on agent 'N/A'
Finished 2025-01-04 05:01:37 with status FAILURE 'Snapshot dependency failed to start: Automated Deployments / Bitcoin ABC Infra / Bitcoin-ABC Infra Checkout'
VCS revisions: 'BitcoinABC_BitcoinAbcStaging' (Git, instance id 22): '6ed06bd778e983cd8ca5b4e9f678ce9ee34cc869' (branch: 'refs/tags/phabricator/diff/51956', checkout rules: '+:. => ./bitcoin-abc')
TeamCity URL https://build.bitcoinabc.org/buildConfiguration/BitcoinABC_BitcoinAbcStaging/881129 
TeamCity server version is 2024.12 (build 174331), server timezone: GMT (UTC)

[05:01:30]W: bt15 (7s)
[05:01:30]i: TeamCity server version is 2024.12 (build 174331)
[05:01:30] : Finalize build settings
[05:01:30] : Collecting changes in 2 VCS roots
[05:01:30] :	 [Collecting changes in 2 VCS roots] VCS Root details
[05:01:30] :		 [VCS Root details] "Bitcoin ABC Staging" {instance id=22, parent internal id=3, parent id=BitcoinABC_BitcoinAbcStaging, description: "ssh://vcs@reviews.bitcoinabc.org:2221/source/bitcoin-abc-staging.git#refs/heads/master"}
[05:01:30] :		 [VCS Root details] "abc-infrastructure" {instance id=24, parent internal id=7, parent id=AutomatedDeployments_BitcoinAbcDeveloperTools_AbcInfrastructure, description: "ssh://vcs@reviews.bitcoinabc.org:2221/source/infrastructure.git#refs/heads/master"}
[05:01:30]i: Loading current repository state for VCS root 'Bitcoin ABC Staging' (6s)
[05:01:30]i:	 [Loading current repository state for VCS root 'Bitcoin ABC Staging'] Loading current repository state for VCS root 'abc-infrastructure' (6s)
[05:01:30]i:		 [Loading current repository state for VCS root 'abc-infrastructure'] VCS root 'Bitcoin ABC Staging': git -c credential.helper= -c credential.helper=/opt/teamcity/temp/credHelper11406208710090663430.sh ls-remote origin
[05:01:30]i:		 [Loading current repository state for VCS root 'abc-infrastructure'] VCS root 'abc-infrastructure': git -c credential.helper= -c credential.helper=/opt/teamcity/temp/credHelper1864931400857660944.sh ls-remote origin
[05:01:30]i:		 [Loading current repository state for VCS root 'abc-infrastructure'] VCS root 'Bitcoin ABC Staging': Warning: Permanently added '[reviews.bitcoinabc.org]:2221' (ED25519) to the list of known hosts.
[05:01:30]i:		 [Loading current repository state for VCS root 'abc-infrastructure'] VCS root 'abc-infrastructure': kex_exchange_identification: read: Connection reset by peer
[05:01:30]i:		 [Loading current repository state for VCS root 'abc-infrastructure'] VCS root 'abc-infrastructure': Connection reset by 51.161.87.173 port 2221
[05:01:30]i:		 [Loading current repository state for VCS root 'abc-infrastructure'] VCS root 'abc-infrastructure': fatal: Could not read from remote repository.
[05:01:30]i:		 [Loading current repository state for VCS root 'abc-infrastructure'] VCS root 'abc-infrastructure': 
[05:01:30]i:		 [Loading current repository state for VCS root 'abc-infrastructure'] VCS root 'abc-infrastructure': Please make sure you have the correct access rights
[05:01:30]i:		 [Loading current repository state for VCS root 'abc-infrastructure'] VCS root 'abc-infrastructure': and the repository exists.
[05:01:31]i: Waiting for completion of current operations for the VCS root 'Bitcoin ABC Staging'
[05:01:36]i: Detecting changes in VCS root 'Bitcoin ABC Staging' (used in 'Diff Testing', 'Staging Checkout Dummy')
[05:01:36]i: Will collect changes for 'Bitcoin ABC Staging' starting from revision 26bacf0d4516e7c64a648de2ac8fa1b487416670
[05:01:36] : Compute revision for 'Bitcoin ABC Staging'
[05:01:36] :	 [Compute revision for 'Bitcoin ABC Staging'] Upper limit revision: 6ed06bd778e983cd8ca5b4e9f678ce9ee34cc869
[05:01:36]i:	 [Compute revision for 'Bitcoin ABC Staging'] MaxModId = 77342
[05:01:36] :	 [Compute revision for 'Bitcoin ABC Staging'] Computed revision: 6ed06bd778e983cd8ca5b4e9f678ce9ee34cc869
[05:01:37]W: Build was removed from the queue with comment: This build has not been started because some of the builds it depends on failed to start

back out npm deps used in testing

back out --forbid-only removal

back out package-lock.json changes not related to version bump

clean up comments in global.d.ts to explain what we are actually doing with this file

Harbormaster completed remote builds in B31914: Diff 51957.Sat, Jan 4, 05:09

Harbormaster completed remote builds in B31915: Diff 51958.Sat, Jan 4, 05:13

bytesofman edited the test plan for this revision. (Show Details)Sat, Jan 4, 05:14

bump ecash-agora

Harbormaster completed remote builds in B31916: Diff 51959.Sat, Jan 4, 05:19

Harbormaster completed remote builds in B31917: Diff 51960.

Harbormaster completed remote builds in B31918: Diff 51961.Sat, Jan 4, 05:26

bytesofman published this revision for review.Sat, Jan 4, 05:31

bytesofman added inline comments.

modules/ecash-lib/src/ecc.ts
18 ↗	(On Diff #51960)	I was not able to test this. I tried to do it in this repo by testing ecc before calling `initWasm`, but it still worked as expected. When I try to test the repo as a dep in other repos, I still get `ecc is not a constructor` Mb we should remove it. However, the reason I chose this approach is because typescript would not build if Ecc can possibly not have a constructor method. This class solved that ts issue. It might not be doing anything else. But that does not really change current behavor of Ecc not working before initWasm.

update

this (may) only work sometimes. Running into tests that passed last night fail this morning ... can't get them to work now... cache issue? mistake in setup and versions installed last night? not sure. tough to test as the problem only occurs in npm-installed versions

could still use some review here since ... mb I am missing something, mb this actually breaks stuff

tobias_ruck requested changes to this revision.Sat, Jan 4, 14:14

tobias_ruck added a subscriber: tobias_ruck.

tobias_ruck added inline comments.

modules/ecash-lib/src/ecc.ts
17	The better abstraction is probably an Ecc object that proxies method calls to WASM, and throws an error if not initialized yet when the method is called. That might also fix the issue

This revision now requires changes to proceed.Sat, Jan 4, 14:14

note that rc4 does still "work" with

const myNewEcc = new (globalThis as unknown as GlobalThis).ecashlib.Ecc();

but new Ecc() gives the (existing behavior) "not a constructor" error

use a Proxy to get Ecc to work with existing syntax across dependency levels

This one seems to be working

available on npm as ecash-lib@1.2.2-rc16

I have not used a Proxy before and I am not totally sure this is the right implementation. However it is passing in my local tests, and console.log shows it is behaving as expected --

(after initWasm() in "parent" app)

console.log(globalThis.ecashlib)
{
  Ecc: [class Ecc],
  HASHES: {
    sha256: [Function (anonymous)],
    sha256d: [Function (anonymous)],
    shaRmd160: [Function (anonymous)]
  }
}

console.log(globalThis.ecashlib.Ecc)
[class Ecc]

console.log(Ecc)
Ecc [class EccProxy]

.... mb I still have some other cache issue going on in my tests, but at least this seems to be the right approach.

Harbormaster completed remote builds in B31919: Diff 51962.Sat, Jan 4, 23:21

never mind. once I

rm -rf node_modules
rm package-lock.json
npm i

this doesn't work at all. actually get an infinite recursion issue instead, nice.

simplify, use function instead of Proxy

Harbormaster completed remote builds in B31920: Diff 51963.Sun, Jan 5, 05:36

teamcity added a comment.Sun, Jan 5, 05:36

This comment was removed by Fabien.

mb it was actually working at rc-16 but this one is simpler anyway. got better at ensuring the proper version was being tested iterating through various other attempts.

was running into mystery cache issues in testing, with testing of new versions sometimes not really testing the "new" version

what worked: instead of pinning versions or npm i the new version, type it exactly, e.g. "ecash-lib": "1.2.2-rc19", not "ecash-lib": "^1.2.2-rc19",

this one worked after lots of rm -rf node_modules && rm package-lock.json && npm i ... also worked with ecash-agora installing this version of ecash-lib (its use of shaRmd160 works if the parent app has called initWasm)

could still be better ways to do this.

teamcity added a comment.Sun, Jan 5, 05:40

This comment was removed by Fabien.

Fabien added a subscriber: Fabien.Mon, Jan 13, 10:08

tobias_ruck requested changes to this revision.Mon, Jan 13, 10:19

tobias_ruck added inline comments.

modules/ecash-lib/global.d.ts
8 ↗	(On Diff #51963)	why do we need this ts definition? Do we want users to use the globalThis namespace? or is it just to make our own code happy? I'm not too comfortable with the idea that users can just use `globalThis.ecashlib.Ecc` (or maybe even `ecashlib.Ecc`), and changing any of that would be a breaking change
modules/ecash-lib/src/ecc.ts
33 ↗	(On Diff #51963)	Wouldn't it be more convenient (and more expected) to allow users to create an `Ecc` instance, and then it becomes usable after initialization? This way we allow the user to create useless objects that they have to throw away. It seems better to just throw an error instead—or much better to create an Ecc object that upgrades itself after `initWasm` has been called.
modules/ecash-lib/src/hash.ts
11 ↗	(On Diff #51963)	Instead of adding adding all this stuff to global.d.ts, just add a `GlobalThisEcashLib` or `GlobalThisExt` with an `ecashlib` field, and then use that instead of `typeof globalThis`

This revision now requires changes to proceed.Mon, Jan 13, 10:19

remove global.d.ts, remove (most) uses of typeof globalThis

modules/ecash-lib/global.d.ts
8 ↗	(On Diff #51963)	I don't think we can accomplish what we are trying to accomplish in this diff (make Ecc and HASHES available at all levels of the dependency tree from a single top-level `initWasm` call) without adding these methods to `globalThis`. Once they are in `globalThis` ... yeah we cannot prevent users being able to call them in this way, though we would not document it. It is a kind of de facto private method for the class. I added this definition as part of a battle to make typescript happy. There may be some alternative way to do this without a `global.d.ts` file, will see what I can do.
modules/ecash-lib/src/ecc.ts
33 ↗	(On Diff #51963)	Wouldn't it be more convenient (and more expected) to allow users to create an Ecc instance, and then it becomes usable after initialization? Yes -- this is also mostly evidence of a typescript battle. The issue is that I was not able to figure out how to make an Ecc that does this. It seems better to just throw an error instead This is the current behavior. The constructor itself does nothing but throw an error, so the other methods are unreachable. The other methods only exist to make typescript happy about Ecc having a consistent shape. I went down a big rabbit hole of making Ecc have getter and setter methods so that it would "become" usable after initWasm, but I was not able to get it to work. The issue is that these get defined at the time of the export, so even after initWasm they would still be returning the originally exported (useless) Ecc. There really might be a way to do it. I could not figure it out tho. I did not update this diff for the dozen+ `rc` versions I published, but you could check those out on npm to see that approach. Failed approaches `rc10` exports.Ecc = (() => { const globalEcc = globalThis.ecashlib?.Ecc; return globalEcc \|\| EccUninitialized; })(); fails because (I think) it is defined (and runs) at time of export, so even after initWasm adds to globalThis, it doesn't realize this. `rc9` (you can check these by visiting npm, going to "code" tab, then going to `dist/ecc.js`... there are 19 versions) Use a getter to try and make Ecc automatically start working after initWasm() // Now update Ecc to use globalThis for external access (if available) // We use a getter because installing libs could reference Ecc here even after // initWasm() due to module caching Object.defineProperty(exports, 'Ecc', { get: () => { return (globalThis.ecashlib?.Ecc \|\| exports.Ecc); }, set: value => { exports.Ecc = value; if (globalThis.ecashlib) { globalThis.ecashlib.Ecc = value; } else { Object.assign(globalThis, { ecashlib: { ...(globalThis.ecashlib \|\| {}), Ecc: value, }, }); } }, configurable: true, // Allows for future modification }); function __setEcc(ecc) { exports.Ecc = ecc; Object.assign(globalThis, { ecashlib: { ...(globalThis.ecashlib \|\| {}), Ecc: ecc, }, }); } Note that I also added a setter -- this is because I ran into thrown errors about it not having a setter. I went through a few iterations on this approach but also could not get it to work -- I think same reason, things are defined at the time of the export. So -- yes, totally, it would be more convenient and better for it to behave in this way. I put some time into trying to work this out, but was not able to do it.
modules/ecash-lib/src/hash.ts
11 ↗	(On Diff #51963)	this is another ugly result of a battle with typescript import type { Ecc, EcashLibHashes } from '../global'; let HASHES: EcashLibHashes; interface GlobalThisExt extends GlobalThis { ecashlib: { HASHES: EcashLibHashes; Ecc: { new (): Ecc }; }; } export function sha256(data: Uint8Array): Uint8Array { return (globalThis as GlobalThisExt).ecashlib.HASHES.sha256(data); } This gives Conversion of type 'typeof globalThis' to type 'GlobalThisExt' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first. Property 'ecashlib' is missing in type 'typeof globalThis' but required in type 'GlobalThisExt'.ts(2352) I can though do this: export function sha256(data: Uint8Array): Uint8Array { return (globalThis as unknown as GlobalThisExt).ecashlib.HASHES.sha256(data); } ... judgment call if this is better, but yeah, I think it is a bit, it is more readable.

Okay I made some passes at cleaning up

removed the global.d.ts file which was a mess, required redefining things
removed all uses of typeof globalThis except for one, which I was unable to figure out how to lose

Here is how I tested this

Remove all local deps from ecash-lib, replace with npm deps
Bump version to rc21
npm i && npm run build && npm publish --tag rc
Go to ecash-agora and remove local deps, replace with npm deps, including this just-published rc21 of ecash-lib
npm i && npm run build && npm publish --tag rc
Go to my repo that has a wallet class that uses ecash-lib. Install this rc-21. Build. Tests pass (tests call new Ecc() in this class)
Go to my repo that uses this wallet class as a dep. Install ecash-lib rc21 and ecash-agora rc21 (errors are thrown if the ecash-agora version used does not itself use the ecash-lib version used). Tests pass (the tests call initWasm in this parent app, and then call methods that require initWasm in both ecash-agora and in the wallet class that uses ecash-lib)

modules/ecash-lib/src/hash.ts
15 ↗	(On Diff #52165)	I could not figure out how to lose this without this, `npm run build` gives src/hash.ts:15:40 - error TS2552: Cannot find name 'GlobalThis'. Did you mean 'GlobalThisExt'? 15 export interface GlobalThisExt extends GlobalThis { ~~~~~~~~~~ Found 1 error in src/hash.ts:15 I think, though, having it here does make it more readable; it is clear that we are extending `globalThis`.

Harbormaster completed remote builds in B32069: Diff 52165.Mon, Jan 13, 14:34

tobias_ruck added inline comments.Mon, Jan 13, 15:56

modules/ecash-lib/src/ecc.ts
33 ↗	(On Diff #51963)	Just create an object that proxies to ffi.Ecc, if it's initialized. Have an internal object that's `ecc: ffi.Ecc \| undefined`. When calling any method (e.g. derivePubkey), proxy it to `this.ecc`. Otherwise, initialize it if the wasm is ready. If not, throw an error. The constructor should simply create an ffi.Ecc object if the wasm is ready, and if not leave it undefined so it will be initialized later.

bytesofman marked an inline comment as done.Mon, Jan 13, 18:03

bytesofman added inline comments.

modules/ecash-lib/src/ecc.ts
33 ↗	(On Diff #51963)	the problem I ran into with this approach is that the class is defined before Ecc is ready. So it just returns undefined even after `initWasm`, when it "should" be ready -- even if I had the class returning the globalThis (since that was also defined in the class before initWasm ... at least that is my theory on why it wasn't working). Potentially it is possible to do and I was missing some details. But I iterated through lots of attempts for this, including an actual `Proxy`, which is something I didn't even know existed in JS before this. With this recommended approach (specifically using `ffi` as the comment describes) ... not sure how to do this without having distinct EccNode and EccBrowser classes, which would be nice to avoid...tho also mb we want to do that. Would help for some cases e.g. nextjs uses server and client-side JS and currently needs some weird shims to use `ecash-lib`. That said -- mb this is an optimization for another diff? The current behavior already has Ecc simply not working and throwing a mystery error if `initWasm` has not been called. This diff does improve that behavior, as well as resolving the issue of `initWasm` not `inittingWasm` across the whole dependency tree.

Some alt options on this diff

We could move ecash-agora into ecash-lib. agora is becoming more critical to "normal" ecash use, and since it uses so many of the functions here, would make sense to have it here. In this way, apps that use both would not run into the "initWasm() does not init all functions across the dependency tree" issue.
We would also then need to add the planned ecash-wallet class here

imo this is "better" in some ways -- we avoid modifying globalThis for example. However this would also mean that any developer building an ecash library would need to find a way to get it into ecash-lib. I don't think we can maintain this indefinitely, at least not without planning to always have ABC responsible for all future libraries and expansions.

So, I think it's important we do something to make sure libraries that use ecash-lib can have initWasm() methods made available across the dependency tree at the top level.

Another option would be to get rid of webassembly entirely. I'm not personallly clear on the benefits and drawbacks of this approach. Having to initWasm() is certainly a significant complication that can't be totally abstracted. Requires custom modifications to all test environments that use this library, special handling in all apps that use this library. Would be nice if the lib "just worked."

If we want to preserve the use of wasm, the approach outlined in this diff is the best I can think of so far. It allows other libraries to use ecash-lib as a dep but still have wasm methods activated across the full dependency tree at the app level. It also allows continued modular development of other future libraries (for example we will probably have ecash-trollbox as a similar modular chronik-client plugin and lib, a la ecash-agora; we would expect many more extensions and we can't reasonably anticipate getting them all into ecash-lib).

[ecash-lib] attach ecc and hashes to globalThis
Needs ReviewPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 51961

modules/ecash-agora/README.md

modules/ecash-agora/package-lock.json

modules/ecash-agora/package.json

modules/ecash-lib/README.md

modules/ecash-lib/global.d.ts

modules/ecash-lib/package-lock.json

modules/ecash-lib/package.json

modules/ecash-lib/src/ecc.ts

modules/ecash-lib/src/hash.ts

modules/ecash-lib/tsconfig.json

[ecash-lib] attach ecc and hashes to globalThisNeeds ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 51961

modules/ecash-agora/README.md

modules/ecash-agora/package-lock.json

modules/ecash-agora/package.json

modules/ecash-lib/README.md

modules/ecash-lib/global.d.ts

modules/ecash-lib/package-lock.json

modules/ecash-lib/package.json

modules/ecash-lib/src/ecc.ts

modules/ecash-lib/src/hash.ts

modules/ecash-lib/tsconfig.json

[ecash-lib] attach ecc and hashes to globalThis
Needs ReviewPublic
Actions

Revision Contents
Changeset List