refactor(experimental): errors: `addresses` package #2187

buffalojoec · 2024-02-24T19:53:10Z

Adds custom SolanaError throws to the @solana/addresses package.

buffalojoec · 2024-02-24T19:53:16Z

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

packages/errors/src/context.ts

steveluscher

I'd rather have these all as separate error codes. You can always catch the SolanaError without caring what type it is.

try {
    const pda = await createProgramDerivedAddress(/* ... */);
} catch(e) {
    if (isSolanaError(e)) {
        // bad thing
    } else {
        throw e;
    }
}

packages/addresses/src/address.ts

packages/addresses/src/program-derived-address.ts

packages/errors/src/context.ts

steveluscher · 2024-02-26T05:29:29Z

I see that this sub-error thing happens in later PRs too. Listen, one thing we could do is to to make use of Error#cause. We'd have to extend SolanaError somehow, maybe by adding cause to the context.

throw new SolanaError(SOLANA_ERROR__PDA_SUCKS, {
    cause: new SolanaError(
        SOLANA_ERROR__PDA_SPECIFICALLY_SUCKS_BECAUSE_OF_THIS,
        { /* context for cause */ },
    ),
    /* other context */
});

Then people can be like:

try {
    const pda = await createProgramDerivedAddress(/* ... */);
} catch (e) {
    if (isSolanaError(e, SOLANA_ERROR__PDA_SUCKS)) {
        const why = e.cause;
        // Do something with `cause`, or not.
    } else {
        throw e;
    }
}

I don't love it though. If we can get away without doing that, we probably should. If there are good use cases for sub-errors, then we should lean on cause.

Notes:

You'll have to enable ES2022.Error in tsconfig.json to be able to use cause
super(message, { cause });
TypeScript typing will probably be super tough; you'll have to delete the cause property from context and write some type tests.

buffalojoec · 2024-02-27T07:32:31Z

Alright I think I got the message, guys.

steveluscher

steveluscher · 2024-02-27T17:46:13Z

packages/addresses/src/address.ts

+            throw new SolanaError(SOLANA_ERROR__INCORRECT_BASE58_ADDRESS_LENGTH, {
+                actualLength: putativeAddress.length,
+            });


When I wrote this error it was intended to be a fast-path check on the number of bytes, without actually counting the number of bytes. Like, looking at the string length of the base 58 string is enough to tell you that it's definitely not 32 bytes, without knowing exactly how many bytes you're dealing with.

This, all to say that I think this error and the one below it are meant to be the same one, and that it's not actually interesting to know how long the base 58 string is.

Maybe make them the same error, and make actualLength: number | 'UNKNOWN'?

I don't think this is a bad check, and the code is (somewhat) informative, but I can see how the two errors could be merged. I'm not really a huge fan of actualLength: 'UNKNOWN', and I certainly don't think we should decode the string just to return the real actual byte length, which could invoke another unrelated error.

I think where I'm hesitant is "what do I do if my string is < 32 or > 44"?

SOLANA_ERROR__WHERE_DID_YOU_EVEN_GET_THIS

steveluscher · 2024-02-27T17:47:32Z

packages/addresses/src/address.ts

        }
        // 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 Error(`Expected input string to decode to a byte array of length 32. Actual length: ${numBytes}`);
+            throw new SolanaError(SOLANA_ERROR__INCORRECT_BASE58_ADDRESS_BYTE_LENGTH, {


I'm not sure that it's important, at this point, that the address used to be base58 encoded. The error is that the address itself has the wrong byte length.

Suggested change

throw new SolanaError(SOLANA_ERROR__INCORRECT_BASE58_ADDRESS_BYTE_LENGTH, {

throw new SolanaError(SOLANA_ERROR__INCORRECT_ADDRESS_BYTE_LENGTH, {

It really only makes sense to include the _BASE58 to match the pattern of the error code above it, if it remains in the set.

steveluscher · 2024-02-27T17:58:48Z

packages/errors/src/context.ts

+        index: number;
+        maxSeedLength: number;


I'm trying to think of what I would do with this error. It basically tells me that ‘some seed in this call’ was too long, but not how long it was, nor what the seed was. Also, it makes a value the maximum seed length that probably won't change in our lifetimes, at least not without a code change being necessary in web3.js.

On the other hand, inlining the seed into the error is maybe a bad idea (privacy?).

I'm struggling to think of what this error context could contain that would actually help me fix the bug. Maybe the actual length of the violating seed is a start?

In any case, it's an important question to ask for each error: ‘does this context help the developer find the bug.’

Also, it makes a value the maximum seed length that probably won't change in our lifetimes, at least not without a code change being necessary in web3.js.

Yeah, but I really didn't want to have to introduce two areas where we'd have to change that value, if we ever needed to update the code.

On the other hand, inlining the seed into the error is maybe a bad idea (privacy?).

Probably safe to avoid this, yeah.

I'm struggling to think of what this error context could contain that would actually help me fix the bug. Maybe the actual length of the violating seed is a start?

Index and actual length should do, no? I doubt we need more than that.

Btw, the MAX_SEEDS error gets thrown by createProgramAddress (also the case in the Rust library as well). This can be confusing to the downstream user, since if they provide 16 seeds (MAX_SEEDS), they'll actually be feeding createProgramAddress with 17 seeds, because the bump is included, and they'll get this error.

With the changes I have locally right now, you'd basically get a message "provided: 17" when you can visibly see you only provided 16. Is a descriptive message good enough to elaborate this fact? Or do we want to work out the "max seeds" concept to actually be 15?

steveluscher · 2024-02-29T19:33:19Z

Merge activity

Feb 29, 11:33 AM PST: @steveluscher started a stack merge that includes this pull request via Graphite.
Feb 29, 11:37 AM PST: Graphite rebased this pull request as part of a merge.
Feb 29, 11:38 AM PST: @steveluscher merged this pull request with Graphite.

#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.

github-actions · 2024-03-02T11:43:40Z

🎉 This PR is included in version 1.90.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions · 2024-03-19T08:03:42Z

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.

buffalojoec requested review from lorisleiva and steveluscher February 24, 2024 19:53

buffalojoec mentioned this pull request Feb 24, 2024

refactor(experimental): errors: remove cyclical dependency #2184

Merged

This was referenced Feb 24, 2024

refactor(experimental): errors: context: sort keys #2185

Merged

refactor(experimental): errors: accounts package #2186

Merged

refactor(experimental): errors: assertions package #2188

Merged

mergify bot added the community label Feb 24, 2024

buffalojoec force-pushed the 02-24-refactor_experimental_errors_accounts_package branch from d8ebcf5 to 399694b Compare February 24, 2024 19:56

buffalojoec force-pushed the 02-24-refactor_experimental_errors_addresses_package branch from 1b48e25 to 91b4a68 Compare February 24, 2024 19:56

lorisleiva requested changes Feb 25, 2024

View reviewed changes

packages/errors/src/context.ts Outdated Show resolved Hide resolved

packages/errors/src/context.ts Outdated Show resolved Hide resolved

packages/errors/src/context.ts Outdated Show resolved Hide resolved

steveluscher requested changes Feb 26, 2024

View reviewed changes

packages/addresses/src/address.ts Outdated Show resolved Hide resolved

packages/addresses/src/program-derived-address.ts Outdated Show resolved Hide resolved

packages/errors/src/context.ts Outdated Show resolved Hide resolved

packages/errors/src/context.ts Outdated Show resolved Hide resolved

steveluscher linked an issue Feb 26, 2024 that may be closed by this pull request

[experimental] Convert all thrown errors to SolanaError #2118

Closed

buffalojoec force-pushed the 02-24-refactor_experimental_errors_accounts_package branch from 399694b to f32df50 Compare February 27, 2024 07:32

buffalojoec force-pushed the 02-24-refactor_experimental_errors_addresses_package branch from 91b4a68 to 2bf93b9 Compare February 27, 2024 07:32

buffalojoec requested review from lorisleiva and steveluscher February 27, 2024 07:32

This was referenced Feb 27, 2024

refactor(experimental): codecs: refactor message out of codec assertions #2200

Merged

refactor(experimental): codecs: use bigint for range values on u64 and u128 #2201

Merged

refactor(experimental): errors: compat package #2202

Merged

steveluscher approved these changes Feb 27, 2024

View reviewed changes

lorisleiva mentioned this pull request Feb 27, 2024

[do not merge] error name feedback #2205

Closed

buffalojoec force-pushed the 02-24-refactor_experimental_errors_accounts_package branch from f32df50 to a3b445e Compare February 28, 2024 16:30

buffalojoec force-pushed the 02-24-refactor_experimental_errors_addresses_package branch from 2bf93b9 to b10a9e8 Compare February 28, 2024 16:30

steveluscher force-pushed the 02-24-refactor_experimental_errors_accounts_package branch from a3b445e to 1bc1537 Compare February 29, 2024 19:28

steveluscher force-pushed the 02-24-refactor_experimental_errors_addresses_package branch from b10a9e8 to bb22886 Compare February 29, 2024 19:28

steveluscher force-pushed the 02-24-refactor_experimental_errors_accounts_package branch from 1bc1537 to 8608dc7 Compare February 29, 2024 19:35

Base automatically changed from 02-24-refactor_experimental_errors_accounts_package to master February 29, 2024 19:36

refactor(experimental): errors: addresses package

16ce665

steveluscher force-pushed the 02-24-refactor_experimental_errors_addresses_package branch from bb22886 to 16ce665 Compare February 29, 2024 19:36

steveluscher merged commit bea19d2 into master Feb 29, 2024
6 checks passed

steveluscher deleted the 02-24-refactor_experimental_errors_addresses_package branch February 29, 2024 19:38

steveluscher mentioned this pull request Mar 1, 2024

Refine Address error types #2225

Merged

steveluscher added a commit that referenced this pull request Mar 1, 2024

Refine Address error types (#2225)

f054d90

#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.

github-actions bot added the released label Mar 2, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(experimental): errors: `addresses` package #2187

refactor(experimental): errors: `addresses` package #2187

buffalojoec commented Feb 24, 2024

buffalojoec commented Feb 24, 2024 •

edited

steveluscher left a comment

steveluscher commented Feb 26, 2024

buffalojoec commented Feb 27, 2024

steveluscher left a comment

steveluscher Feb 27, 2024

buffalojoec Feb 27, 2024 •

edited

steveluscher Feb 27, 2024

steveluscher Feb 27, 2024

buffalojoec Feb 27, 2024

steveluscher Feb 27, 2024

buffalojoec Feb 27, 2024

buffalojoec Feb 28, 2024

steveluscher commented Feb 29, 2024 •

edited

github-actions bot commented Mar 2, 2024

github-actions bot commented Mar 19, 2024

	throw new SolanaError(SOLANA_ERROR__INCORRECT_BASE58_ADDRESS_BYTE_LENGTH, {
	throw new SolanaError(SOLANA_ERROR__INCORRECT_ADDRESS_BYTE_LENGTH, {

refactor(experimental): errors: addresses package #2187

refactor(experimental): errors: addresses package #2187

Conversation

buffalojoec commented Feb 24, 2024

buffalojoec commented Feb 24, 2024 • edited

steveluscher left a comment

Choose a reason for hiding this comment

steveluscher commented Feb 26, 2024

buffalojoec commented Feb 27, 2024

steveluscher left a comment

Choose a reason for hiding this comment

steveluscher Feb 27, 2024

Choose a reason for hiding this comment

buffalojoec Feb 27, 2024 • edited

Choose a reason for hiding this comment

steveluscher Feb 27, 2024

Choose a reason for hiding this comment

steveluscher Feb 27, 2024

Choose a reason for hiding this comment

buffalojoec Feb 27, 2024

Choose a reason for hiding this comment

steveluscher Feb 27, 2024

Choose a reason for hiding this comment

buffalojoec Feb 27, 2024

Choose a reason for hiding this comment

buffalojoec Feb 28, 2024

Choose a reason for hiding this comment

steveluscher commented Feb 29, 2024 • edited

Merge activity

github-actions bot commented Mar 2, 2024

github-actions bot commented Mar 19, 2024

refactor(experimental): errors: `addresses` package #2187

refactor(experimental): errors: `addresses` package #2187

buffalojoec commented Feb 24, 2024 •

edited

buffalojoec Feb 27, 2024 •

edited

steveluscher commented Feb 29, 2024 •

edited