-
Notifications
You must be signed in to change notification settings - Fork 14
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
Update common error message #462
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking suggestion!!
@@ -172,7 +172,7 @@ export function pointDecode(point: Uint8Array): JsonWebKey { | |||
point.length !== uncompressedLength | |||
) { | |||
throw new Error( | |||
"Invalid length: point is not in compressed or uncompressed format" | |||
"Invalid API key: Ensure that you are using a valid public and private key for your API key" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
This is def a more helpful message than the old one, but maybe we could include some of the old stuff because it points the user in the direction of the fact that the formatting of the API key is what is off.
Something like:
Invalid API key: Ensure that you are using a valid public and private key for your API key, provided in either the compressed or uncompressed point format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've incorporated some of the original error message in the new message
@@ -172,7 +172,7 @@ export function pointDecode(point: Uint8Array): JsonWebKey { | |||
point.length !== uncompressedLength | |||
) { | |||
throw new Error( | |||
"Invalid length: point is not in compressed or uncompressed format" | |||
"Invalid API key: Ensure that you are using a valid public and private key for your API key" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to make your life more difficult but I don't think this message should be updated. We're in src/tink/elliptic_curves.ts
which deals with generic crypto primitives. It's also a vendored file which shouldn't be touched unless we really need to.
My proposal: let's look at the callers of pointDecode
, catch errors, and re-throw with a different message?
For example, when this fails:
sdk/packages/sdk-browser/src/utils.ts
Line 34 in d9a0465
const jwk = pointDecode(targetKeyBytes); |
This isn't related to API keys. It's because the targetPublicKey is invalid. So you can do something like:
try {
const jwk = pointDecode(targetKeyBytes);
} catch (e: Error) {
if (e.message === "Invalid length: point is not in compressed or uncompressed format") {
// throw a more helpful error with context
throw new Error(`target public key is not a valid compressed public key: ${targetPublicKey}`);
} else {
// rethrow unmodified
throw e
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm yeah, that seems like the way to handle it. I didnt realize this was a vendored file, we should def not change it in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some try/catches to where decodePoint is being called in the code. However I didn't add the conditional to catch just that error message. All of the error messages that are thrown in decodePoint would be pretty unhelpful imo, so I added general error messages that will lead the user to solving their problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interested in conflicting opinions on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what you have in the current diff! Just left a minor rewording suggestion but otherwise, looks solid!
jwk = pointDecode(uint8ArrayFromHexString(compressedPublicKeyHex)); | ||
} catch (e) { | ||
throw new Error( | ||
`invalid API key: Ensure that you are using a valid public and private key in compressed or uncompressed format and they are not switched` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to template the value of compressedPublicKeyHex
in this case?
I think this message should be purely about API public key rather than both public and private keys. Something like unable to load API key: invalid public key ("${compressedPublicKeyHex}")
?
Summary & Motivation
Update a common error message that provides little info as to the root cause
How I Tested These Changes
Did you add a changeset?
If updating one of our packages, you'll likely need to add a changeset to your PR. To do so, run
pnpm changeset
.pnpm changeset
will generate a file where you should write a human friendly message about the changes. Note how this (example) includes the package name (should be auto added by the command) along with the type of semver change (major.minor.patch) (which you should set).These changes will be used at release time to determine what packages to publish and how to bump their version. For more context see this comment.