errors: Lop off the string part of BorshIoError #974
Conversation
#### Problem The `BorshIoError` variant of `InstructionError` was changed from a string to a unit variant in the Rust SDK in [this PR](anza-xyz/solana-sdk#12), but the change hasn't been reflected in Kit yet. #### Summary of changes Change the `BorshIoError` variant to lop off any string that comes back for compatibility with old and new errors emitted by the runtime. Add the new variant to the RPC error type list.
🦋 Changeset detectedLatest commit: b90059f The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
BundleMonFiles updated (4)
Unchanged files (126)
Total files change -149B -0.04% Final result: ✅ View report in BundleMon website ➡️ |
steveluscher
left a comment
steveluscher
left a comment
There was a problem hiding this comment.
I can work on fixing this up.
| }); | ||
| it('produces the correct `SolanaError` for a `BorshIoError` error', () => { | ||
| it('produces the correct `SolanaError` for a `BorshIoError` error from a string', () => { | ||
| const error = getSolanaErrorFromInstructionError(123, { BorshIoError: 'abc' }); |
There was a problem hiding this comment.
This change means that we need to make two tests, right: one that handles the error being { BorshIoError: 'abc' } and a second test that handles it being "BorshIoError"?
There was a problem hiding this comment.
I added it on line 54 as part of the generic test -- shouldn't that do what you're suggesting?
joncinque
force-pushed
the
borshioerror
branch
from
31a2711 to
c5ff14f
Compare
|
Never mind, seems like that only happens with version bumps, so let me know if you want me to include one |
|
Aagh! I said I'd work on it, and now I can't push because you updated the branch, lol. :) |
Ah sorry, I took a guess and assumed you preferred atomic commits over adding commits to PRs 🙏 |
steveluscher
force-pushed
the
borshioerror
branch
from
c5ff14f to
df0f47a
Compare
|
OK, ready to review @joncinque! Did I get everything right? |
| | 'UnsupportedProgramId' | ||
| | 'UnsupportedSysvar' | ||
| | 'BorshIoError' // SDKv3 has a fieldless `BorshIoError` | ||
| | { BorshIoError: string } // SDK pre-v3 has a newtype for `BorshIoError` |
There was a problem hiding this comment.
This is my lack of knowledge of how this is plumbed throughout the system -- if an old block contains { BorshIoError: "string" }, will that cause an issue? Or will it be properly parsed in the errors package?
There was a problem hiding this comment.
I guess it, in fact, depends. Command clicking ‘show references’ on InstructionError eventually leads me to TransactionForAccountsMetaBase which exposes err: TransactionError (which contains InstructionError, sometimes).
I guess the thing that I don't know, is: after the change to the SDK, can RPC methods that return this type (eg. getBlock, blockSubscribe) return the newtype format anymore, or will they always return the unit type of BorshIoError?
There was a problem hiding this comment.
v2 RPCs will always return the newtype, while v3 RPCs will always return the unit.
There was a problem hiding this comment.
Yeah, this would require the thing that we don't have yet, which is the ability to build different Kit bundles based on the version of the RPC and/or enabled features (#28). Options are:
- Scorched Earth, remove the old type, presume that all RPCs will be replace with v3 before anybody who would be affected upgrades to the next version of Kit
- Keep both types, make a plan to eventually remove the old one when v3 is the network.
I figured in this particular case, option one would be fine, but you tell me – how long is it before v3 likely becomes the network?
There was a problem hiding this comment.
v3 currently has the majority of stake (including frankendancer), so maybe scorched earth is ok:
Stake By Version:
3.1.0 - 6 current validators ( 0.15%)
3.0.8 - 1 current validators ( 0.01%)
3.0.7 - 65 current validators ( 4.20%)
3.0.6 - 359 current validators (38.31%) 5 delinquent validators ( 0.00%)
3.0.2 - 1 current validators ( 0.25%)
3.0.1 - 1 current validators ( 0.01%)
2.3.14 - 2 current validators ( 0.03%)
2.3.13 - 25 current validators ( 3.29%) 1 delinquent validators ( 0.02%)
2.3.11 - 63 current validators (16.21%) 2 delinquent validators ( 0.00%)
2.3.10 - 21 current validators ( 2.17%)
2.3.9 - 6 current validators ( 0.10%) 1 delinquent validators ( 0.00%)
2.3.8 - 98 current validators ( 8.51%) 8 delinquent validators ( 0.03%)
2.3.7 - 1 current validators ( 0.04%)
2.3.6 - 59 current validators ( 2.33%) 1 delinquent validators ( 0.00%)
2.3.5 - 2 current validators ( 0.40%)
0.712.30006 - 163 current validators (16.67%)
0.711.30006 - 2 current validators ( 0.19%)
0.709.30000 - 4 current validators ( 0.46%)
0.708.20306 - 50 current validators ( 6.39%)
0.707.20306 - 2 current validators ( 0.23%)
0.1.1 - 0 current validators ( 0.00%) 1 delinquent validators ( 0.00%)
unknown - 0 current validators ( 0.00%) 21 delinquent validators ( 0.01%)
There was a problem hiding this comment.
OK. Let's do it. I need a stamp from you since I'm the last pusher.
You can run |
|
Thanks for the help! |
|
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. |
2 participants
Problem
The
BorshIoErrorvariant ofInstructionErrorwas changed from a string to a unit variant in the Rust SDK in this PR, but the change hasn't been reflected in Kit yet.Summary of changes
Change the
BorshIoErrorvariant to lop off any string that comes back for compatibility with old and new errors emitted by the runtime.Add the new variant to the RPC error type list.