Skip to content
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

Refine Address error types #2225

Merged
merged 1 commit into from
Mar 1, 2024
Merged

Conversation

steveluscher
Copy link
Collaborator

@steveluscher steveluscher commented Mar 1, 2024

#2187 buried these address sub errors in such a way that they'd never reach the caller. This PR allows them to bubble up.

Addresses #2118.

@steveluscher
Copy link
Collaborator Author

steveluscher commented Mar 1, 2024

Comment on lines -77 to 79
} catch (e) {
throw new SolanaError(SOLANA_ERROR__NOT_A_BASE58_ENCODED_ADDRESS, { putativeAddress });
// Fast-path; see if the input string is of an acceptable length.
if (
// Lowest address (32 bytes of zeroes)
putativeAddress.length < 32 ||
// Highest address (32 bytes of 255)
putativeAddress.length > 44
) {
throw new SolanaError(SOLANA_ERROR__ADDRESS_STRING_LENGTH_OUT_OF_RANGE, {
actualLength: putativeAddress.length,
});
}
// Slow-path; actually attempt to decode the input string.
const base58Encoder = getMemoizedBase58Encoder();
const bytes = base58Encoder.encode(putativeAddress);
const numBytes = bytes.byteLength;
if (numBytes !== 32) {
throw new SolanaError(SOLANA_ERROR__ADDRESS_BYTE_LENGTH_OUT_OF_RANGE, {
actualLength: numBytes,
});
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@buffalojoec, I missed this in #2187. This used to aim to create an ‘all up’ error that the address is bad, with the inner error as the cause. Now that we have codes and separate errors for each cause, I don't think we need this.

Also, #2187 buried the sub errors so that they'd never reach the caller.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I see. Nice catch! So can't NOT_A_BASE_58_ADDRESS come out then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That one would have bubbled up, yes, but the others would never have seen the light of day.

I think in other PRs we consolidated on throwing granular errors, rather than one error with a cause.

Comment on lines +19 to +20
export const SOLANA_ERROR__ADDRESS_BYTE_LENGTH_OUT_OF_RANGE = 13 as const;
export const SOLANA_ERROR__ADDRESS_STRING_LENGTH_OUT_OF_RANGE = 14 as const;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Massaged these names while I was here. tl;dr I'm converting blockhash.ts and it's exactly the same as address.ts.

@steveluscher steveluscher force-pushed the 03-01-Refine_Address_error_types branch from 1d75ecf to 34d5666 Compare March 1, 2024 00:21
Copy link
Collaborator

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice

Comment on lines -77 to 79
} catch (e) {
throw new SolanaError(SOLANA_ERROR__NOT_A_BASE58_ENCODED_ADDRESS, { putativeAddress });
// Fast-path; see if the input string is of an acceptable length.
if (
// Lowest address (32 bytes of zeroes)
putativeAddress.length < 32 ||
// Highest address (32 bytes of 255)
putativeAddress.length > 44
) {
throw new SolanaError(SOLANA_ERROR__ADDRESS_STRING_LENGTH_OUT_OF_RANGE, {
actualLength: putativeAddress.length,
});
}
// Slow-path; actually attempt to decode the input string.
const base58Encoder = getMemoizedBase58Encoder();
const bytes = base58Encoder.encode(putativeAddress);
const numBytes = bytes.byteLength;
if (numBytes !== 32) {
throw new SolanaError(SOLANA_ERROR__ADDRESS_BYTE_LENGTH_OUT_OF_RANGE, {
actualLength: numBytes,
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I see. Nice catch! So can't NOT_A_BASE_58_ADDRESS come out then?

@steveluscher steveluscher force-pushed the 03-01-Refine_Address_error_types branch from 34d5666 to b59fecd Compare March 1, 2024 01:05
@steveluscher steveluscher merged commit f054d90 into master Mar 1, 2024
7 of 10 checks passed
@steveluscher steveluscher deleted the 03-01-Refine_Address_error_types branch March 1, 2024 01:47
Copy link
Contributor

github-actions bot commented Mar 2, 2024

🎉 This PR is included in version 1.90.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants