-
Notifications
You must be signed in to change notification settings - Fork 80
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
chore!: use URL-safe alphabet for base64 encoding (ENG-913) #1651
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. I have left some comments on how to get our fork of pbsjon into this crate, with an eye toward pbsjon-types
still using the upstream pbjson
.
I also think we can avoid making the base64 error part of the public API for now.
Cargo.toml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pbjson-types
is listed as a workspace dependency here. Instead of making our fork of pbjson a direct dependency, should we make it a patch pointing to our branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a [patch]
section is ideal, since (at least to me) it implies a temporary workaround, whereas this change is likely to be fairly long-standing. More importantly, patch
es are ignored if they're in deps' manifests, so if we use a patch here, it would be ignored by a crate outside the monorepo depending on e.g. astria-core
.
If you were also meaning we should specify our fork of the pbjson
repo for all the pbjson crates, I'd have less issue with that, assuming we don't do it via a patch :)
@@ -9,7 +9,7 @@ repository = "https://github.com/astriaorg/astria" | |||
homepage = "https://astria.org" | |||
|
|||
[dependencies] | |||
base64 = { workspace = true } | |||
base64 = "0.22.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the base64 encoded blob data we feed into the blob parser command is not controlled by us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, correct - it's from a block explorer.
Closing pending discussion around the scope of changes for this update. |
Summary
This changes all base64-encoding to use the URL-safe alphabet.
Background
We want our base64-encoded data to be URL-safe, and this was not previously the case.
Changes
astria-core-utils
with a single public modulebase64
. This module provides a Display/Debug helper for formatting byte strings while avoiding allocations, and simpleencode
anddecode
functions. Further functions can be provided in the future if required (e.g. encoding to a buffer).base64
crate and oftelemetry::display::base64
have been replaced by using the new functions/type...astria-sequencer-utils
, where the input to the blob-parser will be standard base64. Hence, in that crate only, we still read the input data as standard base64, but all outputs are made using the URL-safe encoding fromastria-core-utils
pbjson
dependency was changed to use a fork, which has the effect of making the generated serde impls of the protobuf types also use the URL-safe alphabet when encoding. (pbjson
already supports decoding from URL-safe-encoded data).Testing
Unit tests were added.
Breaking Changelist
Related Issues
Closes #1649.