This repository was archived by the owner on Jan 22, 2025. It is now read-only.
rpc: rework binary encoding. BREAKING CHANGE#11646
Merged
mvines merged 5 commits intosolana-labs:masterfrom Aug 18, 2020
Merged
rpc: rework binary encoding. BREAKING CHANGE#11646mvines merged 5 commits intosolana-labs:masterfrom
mvines merged 5 commits intosolana-labs:masterfrom
Conversation
mvines
commented
Aug 15, 2020
mvines
force-pushed
the
b64
branch
2 times, most recently
from
f611e0b to
9e532ed
Compare
Codecov Report
@@ Coverage Diff @@
## master #11646 +/- ##
=========================================
- Coverage 82.1% 82.0% -0.1%
=========================================
Files 330 330
Lines 76198 76222 +24
=========================================
+ Hits 62560 62569 +9
- Misses 13638 13653 +15 |
mvines
changed the title
mvines
commented
Aug 16, 2020
| #[serde(rename_all = "camelCase")] | ||
| pub enum UiAccountEncoding { | ||
| Binary, // SLOW! Avoid this encoding | ||
| Binary, // Legacy. Retained for RPC backwards compatibility |
Contributor
Author
There was a problem hiding this comment.
One day we can remove "binary" when it starts getting in the way. It's no longer documented
Comment on lines
-492
to
-500
| if let Some(ref mut account) = rpc_account { | ||
| if let Binary(_) = &account.data { | ||
| let tmp = Binary64(String::new()); | ||
| match std::mem::replace(&mut account.data, tmp) { | ||
| Binary(new_data) => account.data = Binary64(new_data), | ||
| _ => panic!("should have gotten binary here."), | ||
| } | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
The problem with Binary(String) and Binary64(String) is visible here. Serde can't distinguish between the two on deserialization with the the untagged attribute, so this workaround was previously needed for Rust RPC clients.
|
|
||
| let req = format!( | ||
| r#"{{"jsonrpc":"2.0","id":1,"method":"getAccountInfo","params":["{}", {{"encoding":"binary64"}}]}}"#, | ||
| r#"{{"jsonrpc":"2.0","id":1,"method":"getAccountInfo","params":["{}", {{"encoding":"base64"}}]}}"#, |
Contributor
Author
There was a problem hiding this comment.
Ah, this feels so much nicer. "binary64"? What's that! "base64"? Oh, that's base64.
Contributor
Author
|
This PR escalated quickly! It's a breaking change mostly for the newly added binary64 encoding, so the downstream users should still be few and I think the long-term benefits outweigh the minor inconvenience of adapting to the slightly different RPC request/responses |
CriesofCarrots
approved these changes
Aug 17, 2020
Contributor
CriesofCarrots
left a comment
CriesofCarrots
left a comment
There was a problem hiding this comment.
Thanks for wrangling this one! lgtm
This was referenced Aug 18, 2020
mergify Bot
added a commit
that referenced
this pull request
Aug 18, 2020
* Add base64 (binary64) encoding for getConfirmedTransaction/getConfirmedBlock (cherry picked from commit b5f3ced) * decode-transaction now supports binary64 (cherry picked from commit 2ebc68a) # Conflicts: # cli/src/cli.rs * Rework UiAccountData encode/decode such that it works from Rust (cherry picked from commit 757e147) # Conflicts: # account-decoder/src/lib.rs # cli/src/cli.rs * Rename Binary64 to Base64. Establish Base58 encoding (cherry picked from commit adc984a) # Conflicts: # account-decoder/src/lib.rs * Remove "binary" encoding. Document "encoding" as required (cherry picked from commit e528115) * resolve conflicts Co-authored-by: Michael Vines <mvines@gmail.com>
mergify Bot
added a commit
that referenced
this pull request
Aug 18, 2020
* Add base64 (binary64) encoding for getConfirmedTransaction/getConfirmedBlock (cherry picked from commit b5f3ced) # Conflicts: # transaction-status/Cargo.toml * decode-transaction now supports binary64 (cherry picked from commit 2ebc68a) # Conflicts: # cli/src/cli.rs * Rework UiAccountData encode/decode such that it works from Rust (cherry picked from commit 757e147) # Conflicts: # cli/src/cli.rs * Rename Binary64 to Base64. Establish Base58 encoding (cherry picked from commit adc984a) * Remove "binary" encoding. Document "encoding" as required (cherry picked from commit e528115) * resolve conflicts Co-authored-by: Michael Vines <mvines@gmail.com>
mvines
added a commit
that referenced
this pull request
Aug 18, 2020
mvines
added a commit
to mvines/solana
that referenced
this pull request
Aug 18, 2020
…na-labs#11646) (solana-labs#11673)"" This reverts commit fb90fb3.
This was referenced May 23, 2023
This was referenced Oct 13, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problems:
Binary(String)andBinary64(String)enums, requiring RpcClient hacksChanges:
UiAccountDatafor the "base58"/"base64" encoding now return an array of two elements,["encoded data", "encoding type"]instead of just"encoded data". This allows serde_json to work. <-- BREAKING CHANGETODO