Implement hex-to-base64 encoding migration for KeyPackage and Welcome#29
Conversation
WalkthroughAdds configurable content encoding (hex or base64) via MdkConfig/MdkBuilder and a ContentEncoding enum; introduces encode_content/decode_content utilities; makes KeyPackage and MLS Welcome encoding/runtime-aware (tags indicate base64 when used); updates parsing, decoding, and tests accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (2)crates/mdk-core/src/welcomes.rs (2)
crates/mdk-core/src/key_packages.rs (2)
🔇 Additional comments (10)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
❌ Coverage: 86.59% → 86.48% (-0.11%) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/mdk-core/src/welcomes.rs (1)
290-337: Clarify error message and naming now that content may be base64 or hexThe dual‑format
decode_welcome_contentlogic looks correct and backwards‑compatible, but:
- Line 347 still formats
"Error hex decoding welcome event: {:?}"even when the failure is from the base64 path, which can mislead debugging.- Variable name
hex_content(line 344) is now inaccurate since it may hold bytes decoded from either hex or base64.Consider updating both to be format‑agnostic, e.g.:
- let hex_content = match self.decode_welcome_content(&welcome_event.content) { + let content_bytes = match self.decode_welcome_content(&welcome_event.content) { Ok(content) => content, Err(e) => { - let error_string = format!("Error hex decoding welcome event: {:?}", e); + let error_string = format!("Error decoding welcome event content: {:?}", e);This keeps logs accurate while preserving behavior.
Also applies to: 344-364
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/mdk-core/src/groups.rs(2 hunks)crates/mdk-core/src/key_packages.rs(5 hunks)crates/mdk-core/src/lib.rs(4 hunks)crates/mdk-core/src/welcomes.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/mdk-core/src/key_packages.rs (2)
crates/mdk-core/src/lib.rs (1)
create_test_mdk_with_config(313-317)crates/mdk-core/src/groups.rs (1)
relays(155-158)
🔇 Additional comments (6)
crates/mdk-core/src/groups.rs (1)
20-21: Runtime‑selectable welcome encoding is wired correctlyThe welcome rumor encoding cleanly switches between hex and base64 based on
self.config.use_base64_encoding, with clear debug logs on each branch, while preserving hex as the default. This aligns with the phased migration goals without changing call sites or types.Also applies to: 1276-1290, 1293-1299
crates/mdk-core/src/key_packages.rs (2)
984-989: Good separation between “bad encoding” and “bad TLS” in parsing testsThe updated
test_invalid_key_package_parsingnow distinguishes:
- Completely invalid encoding (
"invalid!@#$%") →Error::KeyPackage.- Valid hex with invalid TLS payload (
"deadbeef") →Error::Tls(..).This aligns well with the new decode‑then‑deserialize split and should make debugging upstream issues easier.
2110-2275: Base64/hex encoding tests and size comparison give strong coverageThe new tests around
MdkConfig::use_base64_encodingare well‑designed:
test_key_package_base64_encodingandtest_key_package_hex_encoding_legacyverify that the config flag actually flips the wire format and that both formats round‑trip viaparse_serialized_key_package.test_key_package_cross_format_compatibilityconfirms both “hex MDK” and “base64 MDK” can parse either encoding, which is central to the phased migration story.test_key_package_size_comparisonvalidates the expected ~33% size reduction within a reasonable tolerance band, which is useful as a regression guard.No issues from a correctness standpoint; these tests nicely lock in the intended behavior.
crates/mdk-core/src/lib.rs (3)
42-65: MdkConfig is minimal and appropriate for the current migrationThe
MdkConfigstruct and its helpers (new,with_base64) are straightforward, default to the legacy hex behavior, and clearly document the base64 tradeoff. This is a clean way to carry encoding policy without over‑designing the config surface.
67-156: Builder wiring is correct; be aware of public‑struct field expansion
MdkBuildercorrectly:
- Defaults to
MdkConfig::default()and exposeswith_base64_encodingandwith_configto customize behavior.- Constructs
MDKwithDEFAULT_CIPHERSUITE,SUPPORTED_EXTENSIONS, aRustCryptoprovider, and the chosen config.- Is surfaced via
MDK::builder, withMDK::newdelegating tobuilder(storage).build()to preserve the hex‑by‑default behavior while encouraging the builder for custom configs.One compatibility note: adding
pub config: MdkConfigto the publicMDKstruct (lines 168–180) is a source‑breaking change for any external code constructingMDKvia a struct literal. If the crate aims to preserve that usage, consider:
- Making
confignon‑public (e.g.,pub(crate)with accessor if needed), or- Accepting the semver break and bumping the public version accordingly.
Functionally the implementation looks good; this is mostly about external API expectations.
Also applies to: 168-180, 222-260
300-317: Test helper for custom config is a nice addition
create_test_mdk_with_configcleanly wraps the builder and is already used by the new key‑package tests to drive base64 vs hex behavior. This should make future config‑dependent tests much easier to write.
❌ Coverage: 86.59% → 86.5% (-0.09%) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/mdk-core/src/key_packages.rs (1)
100-153: LGTM!The decode function correctly implements hex-first detection with fallthrough to base64 when hex decode fails. This addresses the robustness concern from the previous review about allowing base64 fallback.
🧹 Nitpick comments (4)
crates/mdk-core/src/welcomes.rs (4)
290-301: Doc comment is misplaced and merged withpreview_welcome's doc.Lines 290-300 describe
decode_welcome_contentbut are incorrectly attached to thepreview_welcomedoc block above. Thepreview_welcomeerror documentation at line 290 abruptly transitions into describingdecode_welcome_content./// # Errors /// -/// Returns an error if both hex and base64 decoding fail. -/// -/// Decodes welcome content from either base64 or hex encoding. -/// -/// Detects the format based on character set: -/// - Hex uses only: 0-9, a-f, A-F -/// - Base64 uses: A-Z, a-z, 0-9, +, /, = -/// -/// If the string contains only hex characters, it attempts hex decoding first (legacy format). -/// If hex decoding fails or if the string contains non-hex characters, it attempts base64 decoding. -/// This provides maximum robustness against edge cases. +/// Returns an error if the welcome message cannot be parsed or processed. + + /// Decodes welcome content from either base64 or hex encoding. + /// + /// Detects the format based on character set: + /// - Hex uses only: 0-9, a-f, A-F + /// - Base64 uses: A-Z, a-z, 0-9, +, /, = + /// + /// If the string contains only hex characters, it attempts hex decoding first (legacy format). + /// If hex decoding fails or if the string contains non-hex characters, it attempts base64 decoding. + /// This provides maximum robustness against edge cases. + /// + /// # Errors + /// + /// Returns an error if both hex and base64 decoding fail. fn decode_welcome_content(&self, content: &str) -> Result<Vec<u8>, Error> {
344-347: Variable name and error message don't reflect dual-format decoding.
hex_contentis misleading since the bytes may have been decoded from base64. Similarly, the error message at line 347 says "Error hex decoding" butdecode_welcome_contenttries both formats.- let hex_content = match self.decode_welcome_content(&welcome_event.content) { + let decoded_content = match self.decode_welcome_content(&welcome_event.content) { Ok(content) => content, Err(e) => { - let error_string = format!("Error hex decoding welcome event: {:?}", e); + let error_string = format!("Error decoding welcome event content: {:?}", e);Also update the usage at line 366:
- let welcome_preview = match self.parse_serialized_welcome(&hex_content) { + let welcome_preview = match self.parse_serialized_welcome(&decoded_content) {
307-336: Decode logic is sound; error message could be more informative.The dual-format detection and fallthrough logic is correct. However, the error at line 333 only includes the base64 error. When hex-only content fails both decodes, including the hex error would help debugging.
fn decode_welcome_content(&self, content: &str) -> Result<Vec<u8>, Error> { let is_hex_only = content.chars().all(|c| c.is_ascii_hexdigit()); + let mut hex_error: Option<String> = None; if is_hex_only { if let Ok(bytes) = hex::decode(content) { tracing::debug!( target: "mdk_core::welcomes", "Decoded welcome using hex (legacy format)" ); return Ok(bytes); } + hex_error = Some("invalid hex".to_string()); tracing::debug!( target: "mdk_core::welcomes", "Hex decode failed for hex-only string, attempting base64" ); } match BASE64.decode(content) { Ok(bytes) => { tracing::debug!( target: "mdk_core::welcomes", "Decoded welcome using base64 (new format)" ); Ok(bytes) } - Err(e) => Err(Error::Welcome(format!( - "Failed to decode welcome as both hex and base64: {}", - e - ))), + Err(e) => { + let msg = match hex_error { + Some(hex_err) => format!( + "Failed to decode welcome: hex error: {}, base64 error: {}", + hex_err, e + ), + None => format!("Failed to decode welcome as base64: {}", e), + }; + Err(Error::Welcome(msg)) + } } }
450-454: Tests correctly verify default hex behavior; consider adding base64 decode test.The existing tests verify hex output which is correct for the default
use_base64_encoding: false. However, there's no test validating thatdecode_welcome_contentcan successfully decode base64-encoded welcome content.Consider adding a test similar to
test_key_package_cross_format_compatibilityin key_packages.rs to verify welcomes can be decoded from both formats.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/mdk-core/src/key_packages.rs(6 hunks)crates/mdk-core/src/welcomes.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/mdk-core/src/key_packages.rs (2)
crates/mdk-core/src/lib.rs (1)
create_test_mdk_with_config(313-317)crates/mdk-core/src/groups.rs (1)
relays(155-158)
🔇 Additional comments (8)
crates/mdk-core/src/welcomes.rs (1)
6-7: LGTM!Standard base64 imports aligned with the key_packages.rs implementation.
crates/mdk-core/src/key_packages.rs (7)
4-5: LGTM!Base64 imports consistent with welcomes.rs.
24-34: LGTM!Documentation correctly describes the dual-format encoding controlled by
MdkConfig::use_base64_encoding. This addresses the previous review feedback.
73-97: LGTM!Clean conditional encoding with appropriate debug logging for traceability.
155-184: LGTM!Clean separation of concerns - decoding is now delegated to the helper function while this method focuses on TLS deserialization and validation.
986-996: LGTM!Test correctly validates that invalid encoding (neither hex nor base64) returns
Error::KeyPackage. The test also keeps the existing hex-valid-but-invalid-MLS test case.
2112-2223: LGTM!Excellent test coverage for the encoding feature:
- Base64 and hex encoding tests verify correct format selection
- Cross-format compatibility ensures both old and new clients can interoperate
- This is critical for the 3-phase migration strategy
2225-2277: LGTM!Good validation of the PR's primary objective. The 25-40% savings range appropriately accounts for base64 padding overhead while confirming the expected ~33% reduction.
❌ Coverage: 86.59% → 86.55% (-0.04%) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/mdk-core/src/welcomes.rs (1)
339-364: Error message inpreview_welcomeis now slightly misleading re: encoding
preview_welcomenow callsdecode_welcome_content, but the error string is still:let error_string = format!("Error hex decoding welcome event: {:?}", e);Given that the helper may decode via hex or base64, consider changing this to a neutral phrasing like
"Error decoding welcome event"(and maybe drop “hex” from the log target message too) so logs remain accurate regardless of the encoding used.crates/mdk-core/src/key_packages.rs (1)
2280-2333: Decode helper tests are good; consider aligning comments/name with actual behaviorThe three tests for
decode_key_package_contentvalidate:
- Non‑hex/base64 (
"!!!") → combined hex+base64 error.- Hex‑only invalid (
"abc") → hex attempt then base64 attempt, with error message confirming both were tried.- Valid hex (
"00000000") → straightforward success.Functionally this is solid and exercises both the base64‑direct path and the hex‑first path (including fallback attempt). Minor nit:
test_decode_fallback_pathand its comment still talk about “fallback path” where hex fails then base64 succeeds, but the body now only covers the simple “valid hex” success path. You might want to either rename the test or tweak the comment to reflect that it’s specifically checking the hex‑success branch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/mdk-core/src/key_packages.rs(6 hunks)crates/mdk-core/src/welcomes.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/mdk-core/src/welcomes.rs (3)
crates/mdk-core/src/lib.rs (1)
create_test_mdk(308-310)crates/mdk-core/src/encrypted_media/manager.rs (1)
create_test_mdk(351-353)crates/mdk-core/src/encrypted_media/crypto.rs (1)
create_test_mdk(204-206)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Coverage
🔇 Additional comments (6)
crates/mdk-core/src/welcomes.rs (2)
290-337: Dual-format welcome decoding helper is sound and aligns with key package behaviorThe
decode_welcome_contenthelper (including its docs) correctly:
- Detects “hex‑only” via
is_ascii_hexdigit.- Prefers hex decoding for legacy welcomes and falls back to base64 on hex failure.
- Uses base64 for any content with non‑hex chars.
- Maps the final failure into a single, clear
Error::Welcome("Failed to decode welcome as both hex and base64: ...").This matches the migration plan and the key‑package decoding strategy, giving good backward compatibility with minimal ambiguity. I don’t see functional issues here.
1105-1156: Welcome decode tests cover invalid and success paths appropriatelyThe three tests for
decode_welcome_contentexercise:
- Non‑hex, non‑base64 input (
"!!!") → combined error.- Hex‑only but structurally invalid (
"abc") → hex attempt then base64 attempt, with error mentioning both formats.- Valid hex (
"00000000") → successful decode.This gives good coverage of the main edge cases for the helper. No changes needed.
crates/mdk-core/src/key_packages.rs (4)
24-28: Config-driven key package encoding looks correct and well-documentedThe updated doc comment and encoding branch in
create_key_package_for_eventcorrectly:
- Describe the content as “encoded” (hex or base64) and tie the choice to
MdkConfig::use_base64_encoding.- Use base64 when the flag is
trueand hex otherwise, with tracing distinguishing “new” vs “legacy” formats.This matches the migration plan and keeps the external API stable (still returns a
String+ tags tuple). Implementation and docs are consistent; no issues here.Also applies to: 71-87, 97-97
100-153: Key package decode helper and parser provide robust dual-format handlingThe new
decode_key_package_contentand updatedparse_serialized_key_package:
- Use the same hex-only detection heuristic as welcomes and try hex first for legacy strings, then base64 on hex failure.
- For non‑hex strings, go straight to base64.
- Map ultimate decode failures into
Error::KeyPackage("Failed to decode key package as both hex and base64: ...").- Keep TLS deserialization failures surfaced as
Error::Tls(..)from the subsequenttls_deserialize.This cleanly separates “cannot decode at all” vs “decoded but not a valid TLS KeyPackage”, which is exactly what the tests expect. There is a very low‑probability corner case where a base64 string composed only of hex characters will be misinterpreted as hex (and then likely fail TLS), but given how unlikely that is and the desire to prefer legacy hex, this tradeoff seems acceptable.
Also applies to: 155-176
986-995: Invalid-encoding test now correctly targets the decode helper pathThe first half of
test_invalid_key_package_parsingnow verifies that a content string that is neither valid base64 nor valid hex ("invalid!@#$%") yields anError::KeyPackage, while a valid-hex but non‑KeyPackage string ("deadbeef") still yieldsError::Tls(..).This neatly validates the new error taxonomy (decode vs TLS failure). Looks good.
2112-2277: Base64/hex mode, cross-format compatibility, and size tests are thoroughThe new tests:
test_key_package_base64_encoding/test_key_package_hex_encoding_legacyassert that:
- The config flag switches the on‑wire encoding.
- Each mode can parse its own output.
test_key_package_cross_format_compatibilityensures both hex- and base64-configured MDKs can parse both formats, which is key for the migration window.test_key_package_size_comparisonmeasures hex vs base64 lengths and asserts a 25–40% size reduction, which is a reasonable band and unlikely to be brittle across minor serialization changes.Collectively these give strong confidence that the config flag works as intended and that mixed ecosystems will interoperate during the rollout.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/mdk-core/src/welcomes.rs (2)
272-301: Doc comments are now attached to the wrong functionThe block starting at Line 272 (“Previews a welcome message…”) currently attaches to
decode_welcome_content, since it’s the next item after the docs. As a result:
decode_welcome_contentis documented as if it were the preview API.preview_welcome(Line 339) has no doc comment anymore.Consider splitting and reordering the docs so each function has an accurate header, e.g.:
- Keep the “Previews a welcome message…” block immediately above
fn preview_welcome.- Add a focused doc block for
decode_welcome_contentdescribing the dual-format decoding and hex-first heuristic.This keeps the public documentation consistent with the actual behavior.
344-363: Update error message and naming now that decoding is hex/base64, not hex-only
preview_welcomenow usesdecode_welcome_content, which may decode either hex or base64, but:
- The variable is still called
hex_content, even when base64 is used.- The error string says
Error hex decoding welcome event, which is misleading for base64 failures and for the dual-format behavior.Consider:
- let hex_content = match self.decode_welcome_content(&welcome_event.content) { + let decoded_content = match self.decode_welcome_content(&welcome_event.content) { @@ - let error_string = format!("Error hex decoding welcome event: {:?}", e); + let error_string = + format!("Error decoding welcome event content (hex/base64): {:?}", e); @@ - let welcome_preview = match self.parse_serialized_welcome(&hex_content) { + let welcome_preview = match self.parse_serialized_welcome(&decoded_content) {This keeps logs and
failure_reasonaccurate as the encoding migrates.
🧹 Nitpick comments (3)
crates/mdk-core/src/welcomes.rs (1)
1105-1188: Align test naming/comments with actual code paths in decode_welcome_contentThe new tests give good coverage of invalid inputs, hex-only odd-length inputs, valid hex, and base64 with padding. One small mismatch:
test_decode_welcome_fallback_pathuses"00000000", which decodes successfully as hex and never exercises the hex-fail→base64 “fallback” path described in the comment.You can either:
- Rename/comment it as a straightforward “valid hex decodes” test, or
- Change the input to a hex-only-but-invalid string that base64 can decode, if you specifically want to exercise the fallback behavior.
Also, the suite duplicates the structure of the key package decode tests; if you find yourself tweaking the heuristic later, consider extracting a small shared helper for the dual-format decode tests to keep them in sync.
crates/mdk-core/src/key_packages.rs (2)
2112-2277: Config-mode, cross-format, and size comparison tests provide strong migration coverageThe new tests around base64/hex modes look good:
test_key_package_base64_encodingverifies that withuse_base64_encoding: true, the content is valid base64 and round-trips throughparse_serialized_key_package.test_key_package_hex_encoding_legacyconfirms the legacy hex path still works when the flag is false.test_key_package_cross_format_compatibilityshows that both hex- and base64-configured MDK instances can parse both encodings, which is exactly what you want during migration.test_key_package_size_comparisondemonstrates the expected ~25–40% size savings from base64 vs hex at the string level.One tiny note: the “not valid hex” assertion in
test_key_package_base64_encodingdepends on the particular serialized key package instance; in practice that’s fine (the probability of a random base64 string of this length being valid hex is astronomically low), but the comment already hints at the nuance. No change strictly required.
2280-2365: Decode tests mirror welcomes; adjust fallback test naming and consider dedupingThe decode tests here parallel those in
welcomes.rsand give good coverage of:
- Non-hex/non-base64 input (
"!!!"),- Hex-only but invalid strings (
"abc"),- Valid hex (
"00000000"),- Base64 with non-hex chars and padding.
As with the welcomes tests,
test_decode_fallback_pathuses"00000000", which succeeds via the hex path and never exercises the “hex fails then base64” fallback described in the comment. Either:
- Rename/comment it as a “valid hex decodes” test, or
- Adjust the input if you genuinely want to hit the fallback branch.
Given the duplication between key package and welcome decoding tests, you might also consider a small shared helper or parametrized test pattern to keep these suites in lockstep when the heuristic changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/mdk-core/src/key_packages.rs(6 hunks)crates/mdk-core/src/welcomes.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
crates/mdk-core/src/key_packages.rs (3)
crates/mdk-core/src/lib.rs (2)
create_test_mdk_with_config(313-317)create_test_mdk(308-310)crates/mdk-core/src/encrypted_media/manager.rs (1)
create_test_mdk(351-353)crates/mdk-core/src/encrypted_media/crypto.rs (1)
create_test_mdk(204-206)
crates/mdk-core/src/welcomes.rs (3)
crates/mdk-core/src/lib.rs (1)
create_test_mdk(308-310)crates/mdk-core/src/encrypted_media/manager.rs (1)
create_test_mdk(351-353)crates/mdk-core/src/encrypted_media/crypto.rs (1)
create_test_mdk(204-206)
🔇 Additional comments (3)
crates/mdk-core/src/welcomes.rs (1)
6-7: Base64 imports line up with new decoding helperThe added
nostr::base64imports match the usage indecode_welcome_contentand keep all base64 operations localized here. No issues.crates/mdk-core/src/key_packages.rs (2)
4-5: Config-driven encoding for key packages looks correct and well-documentedUsing
self.config.use_base64_encodingto switch between:
- hex (legacy, default) and
- base64 (new, smaller) via
BASE64.encodematches the PR’s migration plan, and the updated doc comment now correctly describes that the content may be hex or base64. The added tracing around the chosen encoding format should also make rollout debugging easier.
Also applies to: 24-35, 73-87, 97-97
986-996: Invalid parsing test cleanly distinguishes encoding vs TLS failuresThe updated
test_invalid_key_package_parsingnow:
- Uses a clearly invalid string (
"invalid!@#$%") to assert that encoding errors map toError::KeyPackage.- Uses
"deadbeef"to assert that valid-hex-but-invalid-TLS input surfaces asError::Tls(..).This makes the boundary between “decode failed” and “TLS parse failed” explicit and future-proof.
✅ Coverage: 86.59% → 86.62% (+0.03%) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/mdk-core/src/util.rs (1)
50-62: Dual-format decoding logic is correct; consider enriching hex error detail
is_hex_onlyplusdecode_dual_formatgive the desired behavior: hex-only inputs try hex first and fall back to base64, and non-hex inputs go straight to base64 with accurate messages about which formats were attempted. One optional improvement would be to capture and include the hex decode error in the final message for thehex_onlycase to aid debugging without changing the control flow.Also applies to: 64-110
crates/mdk-core/src/welcomes.rs (2)
271-299: Doc block now mismatchesdecode_welcome_contentand dropspreview_welcomedocsThe
///block starting at Line 271 currently describes previewing a welcome (staged welcome, group data, arguments, etc.) but is attached todecode_welcome_content(the next item) instead ofpreview_welcome. As a result:
preview_welcomehas no doc comment.decode_welcome_contenthas a description that doesn't match its signature or behavior.It would be clearer to restore the preview-specific docs to sit directly above
fn preview_welcomeand givedecode_welcome_contenta focused, shorter doc explaining the dual-format decoding and error semantics.
311-336: Update error message/variable name to reflect dual-format decoding
preview_welcomenow decodes viadecode_welcome_content, but:
- The variable is still named
hex_content.- The error string uses
"Error hex decoding welcome event".This is slightly misleading now that base64 is also supported. Consider renaming the variable to something like
decoded_contentand changing the message to"Error decoding welcome event"(or similar) so logs and failure_reason remain accurate regardless of encoding.crates/mdk-core/src/key_packages.rs (1)
2086-2197: Encoding-mode and cross-format tests look solid; one assertion is theoretically flakyThe new tests:
- Verify base64 encoding when
use_base64_encoding = trueand hex encoding when false.- Confirm both hex- and base64-configured MDK instances can parse both encodings.
One minor point:
test_key_package_base64_encodingasserts thathex::decode(&key_package_str).is_err(). While in practice it's astronomically unlikely that a base64 string for a real key package would consist only of hex characters, this does introduce a tiny theoretical flakiness. If you want to be completely deterministic, you could drop that assertion and rely on the positive checks (valid base64 + successful round-trip parsing) to validate behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/mdk-core/src/key_packages.rs(6 hunks)crates/mdk-core/src/util.rs(2 hunks)crates/mdk-core/src/welcomes.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
crates/mdk-core/src/welcomes.rs (2)
crates/mdk-core/src/util.rs (1)
decode_dual_format(82-110)crates/mdk-core/src/lib.rs (1)
create_test_mdk(308-310)
crates/mdk-core/src/key_packages.rs (2)
crates/mdk-core/src/util.rs (1)
decode_dual_format(82-110)crates/mdk-core/src/lib.rs (2)
create_test_mdk_with_config(313-317)create_test_mdk(308-310)
🔇 Additional comments (10)
crates/mdk-core/src/util.rs (1)
2-3: Base64 engine import/alias is appropriate for shared decodingUsing
nostr::base64::engine::general_purpose::STANDARD as BASE64centralizes the base64 engine choice and keeps callers consistent. No issues here.crates/mdk-core/src/welcomes.rs (3)
13-14: Import ofdecode_dual_formatcleanly centralizes welcome decodingPulling in
decode_dual_formatfromutilkeeps welcome decoding aligned with key package behavior and avoids duplication. No issues.
300-309:decode_welcome_contentcorrectly wrapsdecode_dual_formatwith loggingThe helper cleanly delegates to
decode_dual_format("welcome"), maps the String intoError::Welcome, and logs which format was used. The behavior matches the tests and the migration plan.
1077-1160: New tests give good coverage of dual-format welcome decodingThe added tests exercise:
- Non-hex/non-base64 inputs (base64-only attempt).
- Hex-only invalid inputs (hex attempt then base64 fallback with combined error wording).
- Valid hex path.
- Valid base64 (including padding) where hex is skipped.
This provides solid coverage for the new helper and should catch regressions in the detection/fallback behavior.
crates/mdk-core/src/key_packages.rs (6)
4-6: Imports correctly align key package handling with shared dual-format utilitiesBringing in the base64 engine alias and
decode_dual_format(alongsideNostrTagFormat) keeps key package encoding/decoding consistent with welcomes and the shared util helper. No concerns here.Also applies to: 16-17
24-28: Config-driven encoding increate_key_package_for_eventmatches docs and migration planThe updated docs clearly describe hex vs base64 based on
MdkConfig::use_base64_encoding, and the implementation honors that with a singleencoded_contentbranch plus debug logging for each path. This aligns with the 3‑phase migration strategy and keeps the public behavior well-documented.Also applies to: 34-35, 71-87, 97-98
100-127: Dual-format key package decoding is correct and error taxonomy is clean
decode_key_package_contentcorrectly:
- Delegates to
decode_dual_format("key package")to handle hex-first, then base64 fallback.- Logs which format was used.
- Maps any failure into
Error::KeyPackage, whileparse_serialized_key_packagekeeps TLS deserialization failures asError::Tls.This separation of concerns and error variants is clear and matches the tests’ expectations.
Also applies to: 129-152
960-970: Invalid-encoding parsing test correctly targetsError::KeyPackagepathThe new check for
parse_serialized_key_package("invalid!@#$%")ensuring it yieldsErr(Error::KeyPackage(_))nicely verifies that non-hex/non-base64 inputs fail in the decode layer and never reach TLS parsing. This is the intended behavior.
2200-2251: Size-comparison test correctly demonstrates expected base64 savingsThe size comparison between hex- and base64-encoded key packages, with a 25–40% savings window and a printed percentage, is a good sanity check that the migration yields the expected payload reduction without overfitting to an exact ratio.
2253-2339: Decode helper tests thoroughly cover dual-format edge casesThe new tests for
decode_key_package_contentmirror the welcome tests and exercise:
- Non-hex/non-base64 input.
- Hex-only invalid strings with both hex and base64 attempts.
- Valid hex-only path.
- Base64 inputs (with and without padding) that bypass hex.
These give strong confidence that the helper behaves as intended across edge cases and that error messages match expectations.
✅ Coverage: 86.59% → 86.62% (+0.03%) |
… events This implements a 3-phase migration strategy to reduce payload sizes by ~33%: Phase 1 (Current): - Add dual-format reading support (both hex and base64) - Keep writing hex by default (use_base64_encoding: false) - Backward and forward compatible Phase 2 (After 3 months): - Switch to base64 encoding via config flag - Still read both formats during transition Phase 3 (After 6 months - Optional): - Remove hex support entirely Changes: - Add MdkConfig struct with use_base64_encoding flag (default: false) - Implement smart format detection based on character set - Hex: only [0-9a-fA-F] - Base64: [A-Za-z0-9+/=] - Add conditional encoding in key package and welcome creation - Update builder pattern to support configuration This ensures a smooth migration that handles the 3-month key package lifetime without breaking compatibility.
- Update documentation to mention MdkConfig::use_base64_encoding flag - Add fallback logic: if hex decode fails, try base64 before giving up - Fix clippy warnings about using map over inspect - Improve error messages to indicate both formats were tried This makes the decoder more robust against edge cases while maintaining backward compatibility.
Adds 6 new tests to improve code coverage: - test_decode_invalid_hex_string: Tests non-hex invalid input - test_decode_hex_only_invalid: Tests hex-only but invalid input with fallback - test_decode_fallback_path: Tests valid hex decoding These tests cover the new fallback logic where hex decode failure triggers base64 attempt, addressing the coverage regression.
Adds 4 more tests to increase code coverage: - test_decode_base64_with_special_chars: Tests base64 with non-hex chars - test_decode_base64_with_padding: Tests multiple base64 strings with padding These tests provide comprehensive coverage of the base64 decode path, ensuring all code branches are exercised. Changes: - Removed failing tests that attempted impossible scenarios - Added practical base64 test cases with various padding scenarios
- Extract shared decode_dual_format helper to util.rs - Remove duplicated code from key_packages.rs and welcomes.rs - Fix error messages to accurately reflect which formats were attempted - Update tests to match new error message format
- Move doc comments to correct functions (decode_welcome_content vs preview_welcome) - Rename hex_content to decoded_content for accuracy - Update error message from 'hex decoding' to 'hex/base64' - Rename test_decode_*_fallback_path to test_decode_*_valid_hex for accuracy
2cc6b91 to
d580910
Compare
✅ Coverage: 86.59% → 86.62% (+0.03%) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/mdk-core/src/key_packages.rs (1)
2274-2293: Test may silently pass if base64 decoding succeeds.Same issue as in welcomes.rs - the
if let Err(err)pattern means if"abc"decodes successfully as base64, the test passes without any assertions.Apply the same fix as suggested for
test_decode_welcome_hex_only_invalid:- if let Err(err) = result { - let err_msg = err.to_string(); - assert!( - err_msg.contains("attempted hex and base64"), - "Error should indicate both formats were tried for hex-only string, got: {}", - err_msg - ); - } + let err = result.expect_err("Odd-length hex-only string should fail both hex and base64 decoding"); + let err_msg = err.to_string(); + assert!( + err_msg.contains("attempted hex and base64"), + "Error should indicate both formats were tried for hex-only string, got: {}", + err_msg + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/mdk-core/src/groups.rs(2 hunks)crates/mdk-core/src/key_packages.rs(6 hunks)crates/mdk-core/src/lib.rs(4 hunks)crates/mdk-core/src/util.rs(2 hunks)crates/mdk-core/src/welcomes.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/mdk-core/src/groups.rs
- crates/mdk-core/src/util.rs
🧰 Additional context used
🧬 Code graph analysis (2)
crates/mdk-core/src/welcomes.rs (4)
crates/mdk-core/src/util.rs (1)
decode_dual_format(82-110)crates/mdk-core/src/lib.rs (1)
create_test_mdk(308-310)crates/mdk-core/src/encrypted_media/manager.rs (1)
create_test_mdk(351-353)crates/mdk-core/src/encrypted_media/crypto.rs (1)
create_test_mdk(204-206)
crates/mdk-core/src/key_packages.rs (2)
crates/mdk-core/src/util.rs (1)
decode_dual_format(82-110)crates/mdk-core/src/lib.rs (2)
create_test_mdk_with_config(313-317)create_test_mdk(308-310)
🔇 Additional comments (16)
crates/mdk-core/src/welcomes.rs (4)
13-13: LGTM!Import of shared
decode_dual_formatutility aligns with the DRY principle, avoiding duplication of the detection and decoding logic across modules.
271-297: LGTM!Clean implementation that properly delegates to the shared
decode_dual_formatutility while maintaining module-specific logging and error types.
314-342: LGTM!The integration of dual-format decoding into
preview_welcomeis well-executed. Error messages appropriately mention both hex and base64, and the failure path correctly records the decoding error in theProcessedWelcomestate.
1123-1165: Good test coverage for dual-format decoding.Tests cover the key scenarios: valid hex decoding, base64 with non-hex characters (triggering direct base64 path), and base64 with various padding patterns.
crates/mdk-core/src/lib.rs (4)
42-65: Clean configuration struct with sensible defaults.
MdkConfigis well-documented with clear default behavior (hex encoding for backward compatibility). Thewith_base64()constructor provides a convenient way to enable the new format.
67-156: Well-implemented builder pattern.The
MdkBuilderprovides a clean fluent API with good documentation. The examples in doc comments demonstrate both the default and configured usage paths clearly.
178-179: Backward-compatible API evolution.Adding the
configfield andbuilder()method while keepingMDK::new()functional ensures existing code continues to work. The delegation fromnew()tobuilder().build()maintains a single construction path.Also applies to: 222-260
311-317: LGTM!The new test helper enables testing configuration-dependent behavior while maintaining the existing
create_test_mdk()for tests that don't need custom configuration.crates/mdk-core/src/key_packages.rs (8)
4-5: LGTM!Imports correctly bring in the base64 encoding/decoding capabilities and the shared
decode_dual_formatutility.Also applies to: 16-16
22-48: Doc comments now accurately reflect dual-format support.The updated documentation correctly explains that encoding format depends on
MdkConfig::use_base64_encoding, addressing the previous review feedback.
73-97: LGTM!The encoding logic correctly branches based on configuration, with helpful debug logging to track which encoding path is used.
100-127: LGTM!Clean implementation that properly uses the shared
decode_dual_formatutility, addressing the previous review suggestion to reduce duplication between key package and welcome decoding.
129-158: LGTM!The updated method cleanly separates decoding concerns (now handled by
decode_key_package_content) from TLS deserialization and validation, improving code organization.
2086-2197: Excellent test coverage for encoding configuration and cross-format compatibility.These tests thoroughly validate:
- Base64 encoding when configured
- Hex encoding (legacy) as default
- Cross-format parsing (both MDK instances can read both formats)
This provides strong confidence in the migration path.
2199-2251: Good size comparison test validating the ~33% savings claim.The test explicitly calculates and verifies the size reduction, with reasonable tolerance (25-40%) accounting for encoding overhead variations.
2295-2336: LGTM!The remaining decode tests cover valid hex, base64 with special characters, and base64 with various padding patterns comprehensively.
✅ Coverage: 86.59% → 86.64% (+0.05%) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/mdk-core/src/welcomes.rs (2)
271-297: Dual‑format decoding helper is correct; consider making it stateless/free for reuse
decode_welcome_contentcleanly delegates todecode_dual_format, preserves the label, and logs the chosen format; behavior matches the migration design (hex‑first for hex‑only, else base64). Since the method doesn’t useself, you could optionally make it an associated or free helper (e.g.,fn decode_welcome_content(content: &str)) to signal that it’s pure and make it easier to reuse from other modules without needing anMDKinstance.
319-364: Error handling correctly persists failure state, but nested error formatting and retry semantics deserve a second lookThe decode/parse error branches correctly:
- Mark
ProcessedWelcomeasFailedwith afailure_reason.- Persist that state.
- Log via
tracing::error!.- Return an
Error::Welcomewith the same message.Two minor points to verify/improve:
Nested error message formatting
You format the innerErrorwith{:?}and then wrap it again inError::Welcome:let error_string = format!("Error decoding welcome event content (hex/base64): {:?}", e); // ... return Err(Error::Welcome(error_string));Because
eis already anError, this can yield messages like
Welcome("Error decoding ...: Welcome(\"Failed to decode welcome ...\")").
Using{e}(Display) or propagating the inner message directly would make logs and storedfailure_reasons cleaner and more user‑facing:let error_string = format!("Error decoding welcome event content (hex/base64): {}", e);The same applies to the parse branch (
"Error previewing welcome: {:?}").Non‑retryable state for decode/parse failures
Becausepreview_welcomewrites aProcessedWelcomewith stateFailedandis_welcome_processedonly checks for existence, any subsequentprocess_welcomecalls for the samewrapper_event_idwill treat the welcome as already processed and will not retry decoding/parsing. That’s probably what you want for permanently malformed content, but it does change behavior from “pure preview” to “preview with side effects” and makes decode/parse failures non‑retryable.If that’s intentional, it might be worth a short comment near
is_welcome_processedor the failure branches; if not, you may want to distinguish “failed” vs “processed” in theis_welcome_processedcheck (e.g., only treatingProcessedas terminal).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/mdk-core/src/key_packages.rs(6 hunks)crates/mdk-core/src/welcomes.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/mdk-core/src/key_packages.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/mdk-core/src/welcomes.rs (2)
crates/mdk-core/src/util.rs (1)
decode_dual_format(82-110)crates/mdk-core/src/lib.rs (1)
create_test_mdk(308-310)
🔇 Additional comments (1)
crates/mdk-core/src/welcomes.rs (1)
1081-1163: New decode_welcome_ tests exercise key hex/base64 edge cases well*The new tests cover:
- Non‑hex/non‑base64 input (
"!!!") and assert we report a base64‑decode failure.- Hex‑only but hex‑invalid input (
"abc") and assert the combined “attempted hex and base64” error.- Happy paths for valid hex and base64 (including padding variants).
This gives good confidence that
decode_dual_format’s detection order and error messages are wired correctly throughdecode_welcome_content. Also, the hex‑only‑invalid test now unconditionally asserts an error before inspecting the message, addressing the earlier concern about potentially silent passes.
There was a problem hiding this comment.
Looks great to me!
My only comment is whether it's even possible to correctly detect if something is hex or base64 with 100% accuracy
c0ffeebabe is valid hex, but also valid base64 of course.
perhaps a version tag makes more sense (no tag = hex, v1 = base64)? but I'll leave this as an open question
Implements version-tagged dual-format encoding to support migration from hex to base64 encoding. This eliminates ambiguity where strings like "deadbeef" are valid in both formats but decode to completely different bytes.
✅ Coverage: 86.59% → 86.68% (+0.09%) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/mdk-core/src/groups.rs (1)
28-29: Welcome encoding uses config correctly; consider hoisting encoding out of the loopThe switch to
encode_content(&serialized_welcome, self.config.use_base64_encoding)plus the debug log is correct and aligns with the migration plan.You could optionally compute
encoded_welcomeonce before thefor event in key_package_eventsloop (and log once) sinceserialized_welcomeand the chosen format don’t vary per event, avoiding redundant encoding and log noise.Also applies to: 1275-1294
crates/mdk-core/src/key_packages.rs (1)
951-965: Key package decode tests line up well with the new dual-format utilitiesThe updated tests for parsing and decoding key package content cover the important paths:
test_invalid_key_package_parsingnow correctly expectsError::KeyPackagefor non-hex/non-v1 strings andError::Tlsfor hex that deserializes badly.- The new base64 tests (
test_key_package_base64_encoding, cross-format compatibility, size comparison) verify:
v1:prefixing and valid base64 payloads.- That both hex- and base64-configured MDK instances can parse both formats.
- Expected size savings for base64 vs hex.
- The
decode_key_package_contenttests mirror the welcome decode tests for invalid hex, valid hex, v1 base64 (valid and invalid), and ambiguous strings like"deadbeef".All of this gives strong confidence that the migration will behave as intended for key packages, and that decode semantics match
decode_dual_format. Any remaining duplication between key-package and welcome decode tests is minor and can be refactored later if it becomes noisy.Also applies to: 2081-2396
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/mdk-core/src/groups.rs(2 hunks)crates/mdk-core/src/key_packages.rs(6 hunks)crates/mdk-core/src/util.rs(2 hunks)crates/mdk-core/src/welcomes.rs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
crates/mdk-core/src/welcomes.rs (2)
crates/mdk-core/src/util.rs (1)
decode_dual_format(86-103)crates/mdk-core/src/lib.rs (1)
create_test_mdk(308-310)
crates/mdk-core/src/groups.rs (1)
crates/mdk-core/src/util.rs (1)
encode_content(60-66)
crates/mdk-core/src/key_packages.rs (2)
crates/mdk-core/src/util.rs (2)
decode_dual_format(86-103)encode_content(60-66)crates/mdk-core/src/lib.rs (2)
create_test_mdk_with_config(313-317)create_test_mdk(308-310)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Coverage
🔇 Additional comments (2)
crates/mdk-core/src/util.rs (1)
50-103: Dual-format helpers look correct and symmetric
encode_contentanddecode_dual_formatcleanly implement the versioned"v1:"base64 vs legacy hex split, with clear error messages and a format descriptor for logging. No functional issues spotted here.crates/mdk-core/src/welcomes.rs (1)
319-365: Error handling in preview_welcome is now robust and statefulThe new
preview_welcomeflow correctly:
- Uses
decode_welcome_contentfor dual-format decoding.- On decode or parse failure, creates a
ProcessedWelcomewithFailedstate andfailure_reason, persists it, logs the error, and returnsError::Welcome.This makes processing idempotent and gives callers a clear failure trail without changing the happy path.
Update documentation in key_packages.rs and welcomes.rs to accurately describe the version-tagged dual-format encoding implementation.
✅ Coverage: 86.59% → 86.68% (+0.09%) |
✅ Coverage: 86.59% → 86.65% (+0.06%) |
Replace the v1: content prefix approach with Nostr-native tag-based encoding for KeyPackage and Welcome events. Changes: - Add ["encoding", "base64"] tag when using base64 encoding - Default to hex when no encoding tag present (backward compatible) - Update ContentEncoding enum with from_tags() method - Update all decode methods to use explicit encoding parameter - Update tests for tag-based approach The tag-based approach: - Is more Nostr-native (uses standard tag conventions) - Is backward compatible (unknown tags are ignored by old clients) - Eliminates ambiguity without modifying content
aa182a3 to
30bc334
Compare
✅ Coverage: 86.59% → 86.65% (+0.06%) |
erskingardner
left a comment
There was a problem hiding this comment.
One small thought but definitely don't let that stop you.
| /// This function generates an encoded key package that is used as the content field of a kind:443 Nostr event. | ||
| /// The encoding format is determined by `MdkConfig::use_base64_encoding`: | ||
| /// - When `false` (default): uses hex encoding (legacy format, no encoding tag) | ||
| /// - When `true`: uses base64 encoding with `["encoding", "base64"]` tag (~33% smaller) |
There was a problem hiding this comment.
sorry if you guys have already hashed this out but do we really need the tag? I know detecting base64 vs hex isn't perfect but it seems like we should be able to do it reliably enough.
I don't care that much - we can always remove the tag later once we've cycled out all the hex key packages.
There was a problem hiding this comment.
it depends on the length of the message. If the message is long enough, we can be pretty much 100% sure whether it's hex or base64 (certainty → 100% as length → ∞). But for shorter messages, we have to rely on tricks (like if there's an odd number of characters, it's definitely base64).
for every char there's a 34.4% chance that it's hex and not base64, at 32 chars that's a very low chance, but not zero (on the range of 1e-15).
personally i feel like the tag approach is the best solution if we want to be sure to not break anything, but at the same time, the chances are low enough where it doesn't affect it that much
There was a problem hiding this comment.
There was a problem hiding this comment.
k, cool. let's keep the tag.
jgmontoya
left a comment
There was a problem hiding this comment.
Some comments, let's try to perhaps address them on a future PR
| // Add encoding tag only for base64 (hex is the default, no tag needed) | ||
| if encoding == ContentEncoding::Base64 { | ||
| tags.push(Tag::custom(TagKind::Custom("encoding".into()), ["base64"])); | ||
| } |
There was a problem hiding this comment.
I think we should always push the encoding tag, even when using hex, just for standardness reasons
| /// This function generates a hex-encoded key package that is used as the content field of a kind:443 Nostr event. | ||
| /// This function generates an encoded key package that is used as the content field of a kind:443 Nostr event. | ||
| /// The encoding format is determined by `MdkConfig::use_base64_encoding`: | ||
| /// - When `false` (default): uses hex encoding (legacy format, no encoding tag) |
There was a problem hiding this comment.
Same idea here. The spec is now saying we should include the encoding tag, so we should.
| public_key: &PublicKey, | ||
| relays: I, | ||
| ) -> Result<(String, [Tag; 6]), Error> | ||
| ) -> Result<(String, Vec<Tag>), Error> |
There was a problem hiding this comment.
I think it's better to always include the tag and keep this length fixed
| // Add encoding tag only for base64 (hex is the default, no tag needed) | ||
| if encoding == ContentEncoding::Base64 { | ||
| tags.push(Tag::custom(TagKind::Custom("encoding".into()), ["base64"])); | ||
| } |
| fn decode_key_package_content( | ||
| &self, | ||
| content: &str, | ||
| encoding: ContentEncoding, | ||
| ) -> Result<Vec<u8>, Error> { | ||
| let (bytes, format) = | ||
| decode_content(content, encoding, "key package").map_err(Error::KeyPackage)?; | ||
|
|
||
| tracing::debug!( | ||
| target: "mdk_core::key_packages", | ||
| "Decoded key package using {}", format | ||
| ); | ||
|
|
||
| Ok(bytes) | ||
| } |
There was a problem hiding this comment.
This method seems a bit unnecessary to me, why not call decode_content directly at the calling point?
| "Should NOT have encoding tag when using hex (default)" | ||
| ); | ||
|
|
||
| // Verify we can parse it back with hex encoding |
There was a problem hiding this comment.
Let's try to remove all the comments that don't really add anything or don't clarify anything that isn't already evident by looking at the code
| #[test] | ||
| fn test_key_package_size_comparison() { | ||
| let test_pubkey = | ||
| PublicKey::from_hex("884704bd421671e01c13f854d2ce23ce2a5bfe9562f4f297ad2bc921ba30c3a6") | ||
| .unwrap(); | ||
| let relays = vec![RelayUrl::parse("wss://relay.example.com").unwrap()]; | ||
|
|
||
| // Create with hex | ||
| let hex_config = crate::MdkConfig { | ||
| use_base64_encoding: false, | ||
| }; | ||
| let hex_mdk = crate::tests::create_test_mdk_with_config(hex_config); | ||
|
|
||
| let (hex_key_package, _) = hex_mdk | ||
| .create_key_package_for_event(&test_pubkey, relays.clone()) | ||
| .expect("Failed to create hex key package"); | ||
|
|
||
| // Create with base64 | ||
| let base64_config = crate::MdkConfig { | ||
| use_base64_encoding: true, | ||
| }; | ||
| let base64_mdk = crate::tests::create_test_mdk_with_config(base64_config); | ||
|
|
||
| let (base64_key_package, _) = base64_mdk | ||
| .create_key_package_for_event(&test_pubkey, relays) | ||
| .expect("Failed to create base64 key package"); | ||
|
|
||
| let hex_size = hex_key_package.len(); | ||
| let base64_size = base64_key_package.len(); | ||
|
|
||
| // Base64 should be smaller than hex | ||
| assert!( | ||
| base64_size < hex_size, | ||
| "Base64 ({} bytes) should be smaller than hex ({} bytes)", | ||
| base64_size, | ||
| hex_size | ||
| ); | ||
|
|
||
| // Calculate the savings | ||
| let savings_percent = ((hex_size - base64_size) as f64 / hex_size as f64) * 100.0; | ||
| println!( | ||
| "Size comparison: hex={} bytes, base64={} bytes, savings={:.1}%", | ||
| hex_size, base64_size, savings_percent | ||
| ); | ||
|
|
||
| // Base64 should be approximately 33% smaller (hex is 2x, base64 is 1.33x) | ||
| // Allow some variance due to encoding overhead | ||
| assert!( | ||
| savings_percent > 25.0 && savings_percent < 40.0, | ||
| "Expected savings between 25-40%, got {:.1}%", | ||
| savings_percent | ||
| ); | ||
| } |
There was a problem hiding this comment.
I don't think this test is really useful. This is basically testing a property of the encodings themselves and not our code. I'd just remove it entirely
| #[test] | ||
| fn test_decode_valid_hex() { | ||
| let mdk = create_test_mdk(); | ||
|
|
||
| // Test that valid hex strings decode successfully | ||
| let valid_hex = "00000000"; | ||
| let result = mdk.decode_key_package_content(valid_hex, ContentEncoding::Hex); | ||
| assert!(result.is_ok(), "Valid hex should decode successfully"); | ||
| assert_eq!(result.unwrap(), vec![0, 0, 0, 0]); | ||
| } |
There was a problem hiding this comment.
This test seems a bit redundant given we have the test_key_package_hex_encoding_legacy test
| #[test] | ||
| fn test_decode_base64() { | ||
| let mdk = create_test_mdk(); | ||
|
|
||
| // Test base64 format | ||
| let base64_str = "SGVsbG8="; // "Hello" in base64 | ||
| let result = mdk.decode_key_package_content(base64_str, ContentEncoding::Base64); | ||
|
|
||
| assert!(result.is_ok(), "Should decode valid base64"); | ||
| let decoded = result.unwrap(); | ||
| assert_eq!(decoded, b"Hello"); | ||
| } |
There was a problem hiding this comment.
Same here, it seems like this was already tested no?
| fn decode_welcome_content( | ||
| &self, | ||
| content: &str, | ||
| encoding: ContentEncoding, | ||
| ) -> Result<Vec<u8>, Error> { | ||
| let (bytes, format) = | ||
| decode_content(content, encoding, "welcome").map_err(Error::Welcome)?; | ||
|
|
There was a problem hiding this comment.
I think we could just call the decode_content directly instead of having this additional method which doesn't really seem to be doing a lot
This implements a 3-phase migration strategy to reduce payload sizes by ~33%:
Phase 1 (Current):
Phase 2 (After 3 months):
Phase 3 (After 6 months - Optional):
Changes:
This ensures a smooth migration that handles the 3-month key package lifetime without breaking compatibility.
Summary by CodeRabbit
New Features
Bug Fixes
Public API
Tests
✏️ Tip: You can customize this high-level summary in your review settings.