Skip to content

Implement promises library with non-blocking methods #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tmalahie
Copy link
Contributor

@tmalahie tmalahie commented Dec 7, 2024

This PR adds an alternate version of the wrapper library that supports async operations.
The issue with the current wrapper is that each method is blocking, which is generally an anti-pattern in JS. Concretely it means that when you call a method that takes multiple seconds to complete (like refreshing your assetsà, the whole app will freeze for multiple seconds and you can't do any other operation in between.
I added another module rgb-lib/promises where all methods perform the tasks asynchronously instead and return a promise. It uses worker threads under the hood.

Usage:

const { Wallet, restoreKeys } = require("rgb-lib/promises");
const { WalletData, BitcoinNetwork } = require("rgb-lib");

async function main() {
  const { accountXpub } = await restoreKeys(BitcoinNetwork.Testnet, mnemonic);

  const wallet = await Wallet(new WalletData({
    pubkey: accountXpub,
    // ...
  }));

  const online = await wallet.goOnline(true, "ssl://electrum.iriswallet.com:50013");
  const balance = await wallet.getBtcBalance(online, true);
  
  // etc
}

@zoedberg
Copy link
Member

@tmalahie Sorry for disappearing for a while. Since the solution in #8 is not working I think we can proceed with the solution proposed here. I'm failing to try it though, could you please add a small example that shows how a wallet method should be called?

@zoedberg
Copy link
Member

zoedberg commented Feb 12, 2025

@tmalahie To add more info, I've tried this:

const { Wallet, generateKeys, restoreKeys } = require("./promises");
const { WalletData, BitcoinNetwork } = require("./wrapper");

async function main() {
    let bitcoinNetwork = BitcoinNetwork.Regtest;
    let keys = await generateKeys(bitcoinNetwork);
    console.log("Keys: " + JSON.stringify(keys));

    let restoredKeys = await restoreKeys(bitcoinNetwork, keys.mnemonic);
    console.log("Restored keys: " + JSON.stringify(restoredKeys));

    let walletData = {
        dataDir: "./data",
        bitcoinNetwork: bitcoinNetwork,
        databaseType: DatabaseType.Sqlite,
        maxAllocationsPerUtxo: "1",
        pubkey: keys.accountXpub,
        mnemonic: keys.mnemonic,
        vanillaKeychain: null,
    };
    console.log("Creating wallet...");
    const wallet = await Wallet(new WalletData(walletData));
    console.log("Wallet created");

    const balance = await wallet.getBtcBalance(null, true);
    console.log("BTC balance: " + balance);
}

try {
    main();
} catch (e) {
    console.error("Error running example: " + e);
    process.exit(1);
}

which builds on your usage example. But when executed it returns:

Keys: {"mnemonic":"narrow provide oblige uncover chunk zebra super reopen vacant pupil afford fire","xpub":"tpubD6NzVbkrYhZ4YmN5zBxoPDBKQGUVRYvALPhrn9aVfRZ7hJLYDaFJMsexpC5NbeBUVivMa6Tco4ZpoZmNcPCoMnVR4AikFPgc7Jzad35C61J","accountXpub":"tpubDDoabekrhTqWWJLZ65fYMGeQQa6ckRb6ZfAqNc3jdv4GKsPzY6z7JQcHx6FH7szSFAsoEySq9FymLoRoGtj9efjtDf5cDY7KFyigi2f57ik","accountXpubFingerprint":"ac55bce8"}
Restored keys: {"mnemonic":"narrow provide oblige uncover chunk zebra super reopen vacant pupil afford fire","xpub":"tpubD6NzVbkrYhZ4YmN5zBxoPDBKQGUVRYvALPhrn9aVfRZ7hJLYDaFJMsexpC5NbeBUVivMa6Tco4ZpoZmNcPCoMnVR4AikFPgc7Jzad35C61J","accountXpub":"tpubDDoabekrhTqWWJLZ65fYMGeQQa6ckRb6ZfAqNc3jdv4GKsPzY6z7JQcHx6FH7szSFAsoEySq9FymLoRoGtj9efjtDf5cDY7KFyigi2f57ik","accountXpubFingerprint":"ac55bce8"}
Creating wallet...
Wallet created
node:internal/process/promises:394
    triggerUncaughtException(err, true /* fromPromise */);
    ^

Error: undefined is not a function (calling goOnline)
    at MessagePort.<anonymous> (/home/zoe/work/bitfinex/rgb-lib-nodejs/promises/worker.js:10:19)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:827:20)
    at MessagePort.<anonymous> (node:internal/per_context/messageport:23:28)

Node.js v22.13.1

@tmalahie
Copy link
Contributor Author

Hello @zoedberg thanks for taking time to check it out 🙂
That's weird, your code is working on my end 🤔 I even tried to run it inside docker (using your Dockerfile) to be sure it wasn't an OS discrepency but it also works on that. I get this

> node test-btc.js
Keys: {"mnemonic":"fury sentence pretty note run tiny run avocado swim dance amount inhale","xpub":"tpubD6NzVbkrYhZ4WWjoFVhYkTYmuK3EDsEsdSuvHfa9Nbw5L1WZXeZuTHSnp5WavTBjZZwepYdbPxRPkcADiCeSYW36Er3EFda7wBWPuKdZW1r","accountXpub":"tpubDCB2jorsPHEvwTNgU7sFE3kZhBYikYASXL3pGNWrnKxCoPmtFxGqWy3aMEGDCLmD2pBPY6G8QBEwx2UJAKP5qYyi3bU4tTgC2eRHbe13MTV","accountXpubFingerprint":"a0ba00d9"}
Restored keys: {"mnemonic":"fury sentence pretty note run tiny run avocado swim dance amount inhale","xpub":"tpubD6NzVbkrYhZ4WWjoFVhYkTYmuK3EDsEsdSuvHfa9Nbw5L1WZXeZuTHSnp5WavTBjZZwepYdbPxRPkcADiCeSYW36Er3EFda7wBWPuKdZW1r","accountXpub":"tpubDCB2jorsPHEvwTNgU7sFE3kZhBYikYASXL3pGNWrnKxCoPmtFxGqWy3aMEGDCLmD2pBPY6G8QBEwx2UJAKP5qYyi3bU4tTgC2eRHbe13MTV","accountXpubFingerprint":"a0ba00d9"}
Creating wallet...
BTC balance:  {
  vanilla: { settled: 0, future: 0, spendable: 0 },
  colored: { settled: 0, future: 0, spendable: 0 }
}

I don't know how we can proceed... Maybe try again your code inside docker to see if it works in this case?
You can also try to add logs to understand better what's going on (for example here which is where the error is)
Or we can schedule a call to debug together? I don't want to take too much of your time for this 😕

@zoedberg
Copy link
Member

@tmalahie the link you provided (https://github.com/RGB-Tools/rgb-lib-nodejs/blob/01e36b43388c0244d577c8a552294bdadd7d9996/promises/worker.js#L10) points to an nonexistent commit, not sure it's a just mistake or if you're on a different commit to which I don't have access. Anyway I added a log before the error gets raised:

        console.log("self " + JSON.stringify(self));
        if (typeof self[method] !== "function") {
            throw new Error(
                `${self[method]} is not a function (calling ${method})`,
            );
        }

and it seems self is {"wallet":{"ty":{},"ptr":{}}}, which seems correct.

Since it works on your end I guess we're using different node versions. I'm using node v22.13.1. I've just tried the same code with node v20.18.3 and it seems self is {"wallet":{"ty":{},"ptr":{}}} also in this case, but the script succeeds in calling the getBtcBalance method:

Keys: {"mnemonic":"oyster aim object enter awesome paddle energy forum sustain duty custom horse","xpub":"tpubD6NzVbkrYhZ4YPcL63Nbe8bYTeET16ftByorJY1zM2SYtpyR2R9gmXkdFRxbpKNouWmncdn7KEgK5qEm54i6HzxMUyddZRu9mUZGwuY8fNQ","accountXpub":"tpubDCRv1zbeJZe7cURVSMZpdwuvqu1QrUdz95ido9QKaBugKn5f8zJkB8TJpYHC6r1KgQer3N2yC1MHyp7nvjNr3j8ig41UQVkW9xEuZoNnm2M","accountXpubFingerprint":"3e8e6a85"}
Restored keys: {"mnemonic":"oyster aim object enter awesome paddle energy forum sustain duty custom horse","xpub":"tpubD6NzVbkrYhZ4YPcL63Nbe8bYTeET16ftByorJY1zM2SYtpyR2R9gmXkdFRxbpKNouWmncdn7KEgK5qEm54i6HzxMUyddZRu9mUZGwuY8fNQ","accountXpub":"tpubDCRv1zbeJZe7cURVSMZpdwuvqu1QrUdz95ido9QKaBugKn5f8zJkB8TJpYHC6r1KgQer3N2yC1MHyp7nvjNr3j8ig41UQVkW9xEuZoNnm2M","accountXpubFingerprint":"3e8e6a85"}
Creating wallet...
Wallet created
BTC balance: {"vanilla":{"settled":0,"future":0,"spendable":0},"colored":{"settled":0,"future":0,"spendable":0}}

Since the script wasn't exiting though (I had to press ctrl+c to exit), I modified the call to the main function in this way:

try {
    main().finally(() => {
        process.exit(0);
    });
} catch (e) {
    console.error("Error running example: " + e);
    process.exit(1);
}

and the output is:

Keys: {"mnemonic":"zero surround december cry action chaos thank claim whisper glad elephant torch","xpub":"tpubD6NzVbkrYhZ4YrJzykw34i3DjdRpWtzRjdXYjMtoYJ2dTjUqnmGmu4T7LkJVwX7UDqU55vxZec92f2rMBt4wS9Rvi3LmxphCjxXpg18eLEP","accountXpub":"tpubDCjcwJSjaxB1a5XDSqqSsbpgY5zJi2CM99fY6DBfVp181zzp2ywrCWRQwvmpHWRYTh68NayZ3M6aY2nfv31fdpt6qEv6Kot6dNfQmm2Udcc","accountXpubFingerprint":"73c3cf0d"}
Restored keys: {"mnemonic":"zero surround december cry action chaos thank claim whisper glad elephant torch","xpub":"tpubD6NzVbkrYhZ4YrJzykw34i3DjdRpWtzRjdXYjMtoYJ2dTjUqnmGmu4T7LkJVwX7UDqU55vxZec92f2rMBt4wS9Rvi3LmxphCjxXpg18eLEP","accountXpub":"tpubDCjcwJSjaxB1a5XDSqqSsbpgY5zJi2CM99fY6DBfVp181zzp2ywrCWRQwvmpHWRYTh68NayZ3M6aY2nfv31fdpt6qEv6Kot6dNfQmm2Udcc","accountXpubFingerprint":"73c3cf0d"}
Creating wallet...
Wallet created

as you can see with this change the output of the getBtcBalance call disappears. So in both cases it seems something is not working properly.

Could you please tell me which node version are you using and try the script with node v22? Could you also try to run the script with the modified call to the main function and tell me if you're also not seeing the output of the getBtcBalance in that case? Thank you

@tmalahie
Copy link
Contributor Author

@zoedberg you're right, there was a an issue with my code on node 22. I was testing on node 20.
They changed some logic for inter-process communication in node 22 and my code no longer handled it properly. I pushed some changes to make it more resilient 🙂

About the code not exiting properly it's because of the worker process still running. I added a method clearResources to terminate it cleanly which is better than relying on process.exit

So the new sample becomes

const {
    Wallet,
    clearResources,
    generateKeys,
    restoreKeys,
} = require("./promises");
const { WalletData, BitcoinNetwork } = require("./wrapper");

async function main() {
    let bitcoinNetwork = BitcoinNetwork.Regtest;
    let keys = await generateKeys(bitcoinNetwork);
    console.log("Keys: " + JSON.stringify(keys));

    let restoredKeys = await restoreKeys(bitcoinNetwork, keys.mnemonic);
    console.log("Restored keys: " + JSON.stringify(restoredKeys));

    let walletData = {
        dataDir: "./data",
        bitcoinNetwork: bitcoinNetwork,
        databaseType: DatabaseType.Sqlite,
        maxAllocationsPerUtxo: "1",
        pubkey: keys.accountXpub,
        mnemonic: keys.mnemonic,
        vanillaKeychain: null,
    };
    console.log("Creating wallet...");
    const wallet = await Wallet(new WalletData(walletData));
    console.log("Wallet created");

    const online = await wallet.goOnline(
        true,
        "ssl://electrum.iriswallet.com:50013",
    );
    const balance = await wallet.getBtcBalance(online, true);
    console.log("BTC balance: ", balance);

    await clearResources();
}

main().catch((e) => {
    console.error("Error running example: " + e);
    process.exit(1);
});

Can you test it again with my latest changes and let me know if works this time? 🙌

@zoedberg
Copy link
Member

@tmalahie the example you provided fails because the indexer is for another network, but I commented that part and called the getBtcBalance method without the online object and the script now works, both in node v20 and v22.

Having to call the clearResources method is not ideal though (not sure this is acceptable), could you please look for a solution that frees the worker automatically?

@tmalahie
Copy link
Contributor Author

@tmalahie the example you provided fails because the indexer is for another network, but I commented that part and called the getBtcBalance method without the online object and the script now works, both in node v20 and v22.

Nice! I also had the indexer error, try to use testnet instead of regtest it should work. The error also occurs when using the non-promise methods so it's not an issue with my changes.

Having to call the clearResources method is not ideal though (not sure this is acceptable), could you please look for a solution that frees the worker automatically?

I can't do it unfortunately, nodejs doesn't have the concept of destructors so I can't detect when you no longer needs the wallet. It's the same reason why we have this method wallet.drop() in the current version of the lib that we also have to call manually.

@zoedberg
Copy link
Member

Nice! I also had the indexer error, try to use testnet instead of regtest it should work. The error also occurs when using the non-promise methods so it's not an issue with my changes.

I know how to overcome the issue and that itsn't related to your changes, but I wanted to point out that's good practice to share a working example if the point of the example is to show code is working ok (especially if the code that you want to show to be working is called after the point where the example is failing).

I can't do it unfortunately, nodejs doesn't have the concept of destructors so I can't detect when you no longer needs the wallet. It's the same reason why we have this method wallet.drop() in the current version of the lib that we also have to call manually.

The wallet.drop() is there to overcome an issue related to rust and swig, so I wouldn't say they share the same reason. Anyway, since there are several ways to make blocking code async in nodejs (worker threads are just a way to solve this issue, with its benefits and tradeoffs) and that this solution adds some friction that makes it not ideal, I think we should not merge this PR. It shouldn't be a blocker for anyone since everybody can add worker threads (or whatever other system they like) in the code where they use rgb-lib.

@tmalahie
Copy link
Contributor Author

Well the example using regtest comes from you originally, I didn't want to question it as I thought it was working for you, sorry if you got confused 😕

Anyway ok as you prefer, I still think it would be beneficial to have async behavior out of the box as it's a common practice in node.js. In the end I'm just trying to help you improving the library based on my experience integrating it. But if you don't like it because of minor drawbacks you're the one to decide 🙌

@zoedberg
Copy link
Member

Well the example using regtest comes from you originally, I didn't want to question it as I thought it was working for you, sorry if you got confused 😕

My bad, sorry, I must have done a mistake when pasting the example because locally I was running a different code, without the call to the goOnline method (I'll edit it in order to not leave incorrect code).

Anyway ok as you prefer, I still think it would be beneficial to have async behavior out of the box as it's a common practice in node.js. In the end I'm just trying to help you improving the library based on my experience integrating it. But if you don't like it because of minor drawbacks you're the one to decide 🙌

I also think it would be nice to have an async behavior out of the box but I would like a more standard approach that doesn't need to be documented to be used. I think in the future I'm going to do another attempt with swig and rust (probably exploring also libuv) because fixing the blocking behavior from the source would be the best way to go IMO. Anyway I appreciate your effort, thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants