mainnet-cash/mainnet-js

Suggestion: Avoid using `undefined` as a flag

renovatorruler opened this issue · 0 comments

For instance this is an example from the docs where undefined is being used as a flag.

If you want to get the information about CashToken UTXOs of an address, look up the locked satoshi values, etc., you can do the following call:

const tokenId = undefined;
const utxos = wallet.getTokenUtxos(tokenId);

If tokenId is undefined UTXOs of all token categories will be returned. If tokenId is set, only tokens of this category will be returned. #

A variable in javascript can be undefined because the caller failed to provide it. But here in the method, not providing the tokenId (i.e it is undefined) is taken to mean that the calling method wants ALL the token categories UTXOs.

This can potentially lead to a lot of unfortunate bugs. For instance, imagine if the tokenId was to be received from a database, but somehow that value comes out to be undefined (which makes sense because it was not found, this is the intended behavior). The API above will take it to mean that you must want all the UTXOs, now imagine you wanted to send an token of tokenId:824a366d1de59251f34a02fe1008888fc3386e464cd5e53a885230c5547c2574 but since javascript engine set it to undefined, some other UTXO might be picked up and transaction made (depending upon the other safeguards put up by the developer).

The ‘correct’ value (but not really) to be used is null, because javascript engine will not set a value to be null intentionally. All the places null is visible, it is because the developer set that value.

But we work in crypto and irreversible data structures, we shouldn’t even be using NULL, we should be using explicit flags and values. (one reason is because database uses NULL, which is another entity, the developer doesn’t explicitly control).

It should be:

wallet.getTokenUtxosByTokenId()
wallet.getAllTokenUtxos()

Here's another example from Chris Troutner's minimal-slp-wallet library, demonstrating this issue in a more simple and catastrophic manner:

const mnemonic = localStorage.getItem('mnemonic'); //<value> or null
const bchWallet = new BchWallet(mnemonic); // <value> means restoring the wallet, null means new wallet
await bchWallet.walletInfoPromise 

console.log("Receive funds at: ", bchWallet.walletInfo.cashAddress)

Above, if mnemonic was set to something, getItem would result in a proper value, but if it was not set, either it was set as undefined due to some other bug, or if the value got cleared after the user opened the web page after 6 days on Safari, then it will be NULL, but Chris Troutner's API presumes a null or undefined to mean you want to generate a new wallet.

A random new wallet is generated for the user, and this could lead to a complete loss of funds.