Increase unit test coverage

[mikesposito]

Description

This PR increases the unit test coverage of the controller.

Current change:

branches    80.18 → 90.09  +9.91
functions   93.54 → 96.77  +3.23
lines       91.69 → 97.44  +5.75
statements  91.87 → 97.5  +5.63

Changes

n/a

References

n/a

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[Gudahtt]

LGTM!

[mcmire]

Looks good, thanks for doing this. All of my comments are minor, but it might be nice to be a bit more descriptive with the test names.

[mcmire]

Can we go into a little more detail on what the expected behavior here is? What does "signing" mean? It looks like the input data is encrypted using the seed phrase? Does signMessage on the keyring do most of the work? If so, it might be important to highlight the keyring being used in this test, since the exact keyring being used affects the signing.

[mikesposito]

The data is usually a digest of the message encoded as hex (so it is not encrypted, just hashed with keccak256), ready to be signed with an ecdsa private key, that in our case is handled by the Keyring linked to the from account.

So, the target Keyring must indeed support the signMessage method, and the "real work" is done by it.

Because of this, I would be inclined to change it to forwards the message to the keyring for signing (using signTransaction tests as an example) - but on the other hand this test case doesn't only check if the message gets forwarded to the Keyring: it also checks for the signature correctness, and this is the reason why we use a seed phrase with a known and predictable first-account-address

[mcmire]

Gotcha, yeah that's a bit tricky. Okay, perhaps we can think of a better name later.