LiskArchive / lisk-sdk

🔩 Lisk software development kit
https://lisk.com
Apache License 2.0
2.72k stars 457 forks source link

Offline transaction signing gives a misleading error message #9092

Closed bobanm closed 8 months ago

bobanm commented 8 months ago

Actual behavior

When signing transaction using --offline flag, if there is any error, the real error message is replaced by an incorrect and misleading error:

Application at data path <data-path> is not running.

This makes no sense, because a running node is not needed for offline signing. Even worse, this misleading error makes it very difficult for a user to understand what the real error is.

Expected behavior

Command should return the correct error, which will help the user correct the command input.

Steps to reproduce

While a node is not running, sign transaction offline and provide an invalid value for a flag, e.g. key derivation path:

./bin/run transaction:sign 0a05746f6b656e12087472616e73666572180020c0843d2a20a3f96c50d0446220ef2f98240898515cbba8155730679ca35326d98dcfb680f032340a0804000000000000001080a0b787e9051a14768d592cef5fce076bbd551445cc66a5eeb13393220b676f7420746f6b656e733f3a406fefadbba4d6495a2f632ea2d3a3de405334c974ba77287fb25e25e7a20c46f38c64c0f54069e60a33ee54f3cda7bb10c403d650c78a322b651b0af83cbe2e03 --offline --chain-id 04000000 --key-derivation-path invalid

In this case, the expected error is that key derivation path is invalid.

Which version(s) does this affect? (Environment, OS, etc...)

The latest SDK 6.0 version.

How to fix it

The transaction:sign command contains custom logic which re-throws a custom error if application is not running:

if (error) {
    if (this._dataPath && !isApplicationRunning(this._dataPath)) {
        throw new Error(`Application at data path ${this._dataPath} is not running.`);
    }
    this.error(error instanceof Error ? error.message : error);
}

This does not correctly cover the case when signing is done in offline mode. Furthermore, this custom logic is not even needed. The check if the application is running is already correctly done when getting the API client, and that function throws the correct error:

this._client = await getApiClient(this._dataPath, this.config.pjson.name);

Additional request

Since the concept of a "path" could mean different things, it would be nice to make the invalid key derivation path error message more specific by changing it from Invalid path format into Invalid key derivation path format.