transaction-status-client-types: Fix deserializer for SDKv2#8625
transaction-status-client-types: Fix deserializer for SDKv2#8625joncinque merged 1 commit intoanza-xyz:masterfrom
Conversation
|
Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. |
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8625 +/- ##
=========================================
- Coverage 83.2% 83.2% -0.1%
=========================================
Files 859 859
Lines 373629 373646 +17
=========================================
- Hits 311006 310996 -10
- Misses 62623 62650 +27 🚀 New features to boost your workflow:
|
|
The Cargo.lock changes in this PR are a bit obnoxious because we're bringing in multiple packages of the same name. I could also change the test to just use json values to avoid pulling in multiple versions of the same dependency. @t-nelson what do you think? Is it too much to backport if there's a big Cargo.lock change? |
|
We'll also need to backport #6435 to v2.3 so that v2.3 can deserialize blocks produced by v3.0, mostly while RPCs and clients upgrade. |
|
Why don't fix ser/deser in solana-sdk repo for TransactionError? |
| "solana-instruction", | ||
| "solana-instruction 2.3.0", | ||
| "solana-instruction 3.0.0", | ||
| "solana-message", | ||
| "solana-pubkey", | ||
| "solana-pubkey 3.0.0", | ||
| "solana-reward-info", | ||
| "solana-signature", | ||
| "solana-transaction", | ||
| "solana-transaction-context", | ||
| "solana-transaction-error", | ||
| "solana-transaction-error 2.2.1", | ||
| "solana-transaction-error 3.0.0", |
There was a problem hiding this comment.
these are the only real changes right? rest is just addition of their dep entries and the version added to existing entries for all?
i can probably be fine with this. will give @bw-solana and @sakridge a chance to chime in tho
There was a problem hiding this comment.
Yep that's exactly right. I'm totally fine with inlining the JSON representation of the v2 error though to avoid all this
|
I created PR that should allow to InstructionError process both old and new format for BorshIoError: anza-xyz/solana-sdk#404 If you don't want to use custom Visitor created with macros then probably would be better to check type of value (Object vs String) instead of error based logic. |
Thanks for the call-out, done! I also removed the old dependencies, I think they were going to just be a huge hassle in the long-run, and will make a backport to v3 easier. |
| // This checks compatibility across SDK versions where BorshIoError is | ||
| // a unit variant in v3 and newtype variant in v2. | ||
| let old_error = | ||
| to_value(serde_json::json!({"InstructionError": [0, {"BorshIoError": "Unknown"}]})) |
There was a problem hiding this comment.
won't this lend to breakage should we wind up with a typo here?
There was a problem hiding this comment.
I take your point that we don't get a compile-error from a misspelling here, which we would for InstructionError::BorshIoError. On the flip-side, if the test starts failing because we made a typo, that means the test is doing its job, right?
We'd only get a false positive if we change this string, the one on line 316, but not the one on line 317. While that's possible, I'd argue that it's very unlikely. What do you think?
|
I removed the v2.3 label -- we'll fix the issue at the SDK level for v2 with anza-xyz/solana-sdk#410 For master and v3, I'd like to land this PR, which will avoid bringing |
#### Problem The InstructionError deserializer within UiTransactionError fails to deserialize errors from SDKv2 because `BorshIoError` changed from being a newtype with string to a unit enum variant. #### Summary of changes If deserialization fails, check to see if it's an old `BorshIoError`, and change to the new variation if so. Also include a test using the old and new error formats.
joncinque
force-pushed
the
ixerr-deserializer
branch
from
a469219 to
7ad91b1
Compare
#### Problem The InstructionError deserializer within UiTransactionError fails to deserialize errors from SDKv2 because `BorshIoError` changed from being a newtype with string to a unit enum variant. #### Summary of changes If deserialization fails, check to see if it's an old `BorshIoError`, and change to the new variation if so. Also include a test using the old and new error formats. Co-authored-by: Brian Li <brnli7@gmail.com> (cherry picked from commit ed4cbad)
…ckport of #8625) (#8759) transaction-status-client-types: Fix deserializer for SDKv2 (#8625) #### Problem The InstructionError deserializer within UiTransactionError fails to deserialize errors from SDKv2 because `BorshIoError` changed from being a newtype with string to a unit enum variant. #### Summary of changes If deserialization fails, check to see if it's an old `BorshIoError`, and change to the new variation if so. Also include a test using the old and new error formats. (cherry picked from commit ed4cbad) Co-authored-by: Jon C <me@jonc.dev> Co-authored-by: Brian Li <brnli7@gmail.com>
…#8625) #### Problem The InstructionError deserializer within UiTransactionError fails to deserialize errors from SDKv2 because `BorshIoError` changed from being a newtype with string to a unit enum variant. #### Summary of changes If deserialization fails, check to see if it's an old `BorshIoError`, and change to the new variation if so. Also include a test using the old and new error formats. Co-authored-by: Brian Li <brnli7@gmail.com>
5 participants
Problem
The InstructionError deserializer within UiTransactionError fails to deserialize errors from SDKv2 because
BorshIoErrorchanged from being a newtype with string to a unit enum variant.Summary of changes
If deserialization fails, check to see if it's an old
BorshIoError, and change to the new variation if so.Also includes a test using the old and new SDKs.
Closes #8582 Fixes #8536
Note: the backport will be the exact same thing for v3.0, but v2.3 will change the logic to check for a string
BorshIoErrorand replace it with{"BorshIoError": ""}, and will involve bringing in sdk v3 for the test.cc @BriungRi