PrivateKey.fromString error if key is empty is not developer friendly

[gregscullard]

Description

The following if process.env.PRIVATE_KEY is empty/undefined

const privateKey = PrivateKey.fromString(process.env.PRIVATE_KEY);

throws a TypeError: Cannot read the properties of undefined (reading startsWith) which is due to the fromString input being empty.

Could a check be added for such a condition so that the error is more meaningful to developers ?

Steps to reproduce

const privateKey = PrivateKey.fromString(process.env.NOVALUE);

throws

    const str = text.startsWith("0x") ? text.substring(2) : text;
                     ^

TypeError: Cannot read properties of undefined (reading 'startsWith')

Additional context

No response

Hedera network

other

Version

2.18.1

Operating system

macOS

[janaakhterov]

@gregscullard I'm going to close this issue as not a bug because the way to eliminate this issue is to use TypeScript with your project. The SDK isn't built to take any value as their parameters and that is documented via the type system. It's up to the developer to make sure they're either using TypeScript or they review the documentation. In this particular case PrivateKey.fromString() does not accept null | undefined and hence will not test for either of those options.

[gregscullard]

Reopening because, while I agree with the principle, we deal with developers on a daily basis who encounter this error because their .env is badly configured, and often all they're trying to do is run the examples from within the SDK (which are not in typescript themselves).

One or three lines of code would alleviate this frustration and help developer adoption, nobody likes a cryptic error when first trying to build a hello world.

Are you saying adding the check is technically impossible because PrivateKey is coded in typescript and the parameters to the fromString function are necessarily not null/empty, or that you won't add it out of principle ?

[janaakhterov]

Here are my current thoughts. Sorry they're a bit scattered, but just wanted to write them down.

• Changing the function signature is a breaking change.
• We can implement the change even if we don't update the method signature making this not a breaking change. However, not updating the signature I feel is a bit misleading. Also, if PrivateKey.fromString() silently handles null | undefined what's there to stop someone from creating an issue stating that any *.fromString() method not handling null | undefined is a bug?
• If we proceed with the breaking change, where do we stop with param validation? If PrivateKey.fromString() does JS type validation, why don't any of the setters do param validation?
• If someone is using TS in their project, why must they pay the price of JS param validation?
• Maybe we should instead update the examples to be TS and/or implement JS environment variable validation in the examples directly?
• Maybe we should implement something like PrivateKey.fromEnv() which could handle the this case?

[gregscullard]

I would recommend a non breaking change of course.

I would expect PrivateKey.fromString() not to silently handle null/undefined, but throw a clear exception a developer would understand such as "Cannot recover key from empty string" or such like if the input value is empty, or wrap the underlying call in a try/catch that throws a nicer error message.

[janaakhterov]

I would recommend a non breaking change of course.

I would expect PrivateKey.fromString() not to silently handle null/undefined, but throw a clear exception a developer would understand such as "Cannot recover key from empty string" or such like if the input value is empty, or wrap the underlying call in a try/catch that throws a nicer error message.

The only reason I'm not 100% for this is because the error we're actually catching is something that would never happen if the type system was followed meaning we're still hacking around the type system, and this same principal can be used to implement try/catch on all methods that have parameters as a just in case net.

All-in-all though, I want to state that I do hear you, and I get that for JS devs this is painful, I just wanted to list my issues with this change coming from our use of TS. I'm at a bit of a road block as I want to make it easier for JS devs, but I want a clear line between JS param validation and using TS before proceeding.

cc @rbair23 @SimiHunjan @jg-swirlds What should we do going forward?

[cacampbell]

• This is literally the main use-case for typescript
• We already have extensive, custom JSDocs that are recognized and interpreted by every major editor

The solution, beyond adding checks ad-hoc for runtime errors that people happen to inflict upon themselves, is to re-implement type validation within the library (maybe something like this https://github.com/flix-tech/fp-ts-type-check)

[petreze]

In regards to the mentioned issue, a simple fix itself is not a problem. Basically, in JS we can write

if (!text) {
}

which will evaluate to true if text is one of these:

null undefined NaN empty string ("") 0 false

but having said that, I think that this won't be a good approach because for example:

If we add the following check for PrivateKey.fromString() and somebody runs an example with an empty .env file, it will result in Error: failed to parse entity id: undefined which comes from attempting to get the id with AccountId.fromString(). Therefore, we should add the same error handling there as well.

And the domino effect of this is that, if we add this for PrivateKey, we would have to add handlers for literally every (and not only) .from* method inside the SDK

In conclusion, I would add that an error like TypeError: Cannot read the properties of undefined (reading startsWith) is enough self-explanatory for a developer to recognize where the issue comes from. Especially when there is a line in the console after encountering the error pointing the exact variable which is undefined and the exact line of the example where the error occurs

cc: @gregscullard