Replace serde_cbor with minicbor#784
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces direct serde_cbor usage with minicbor across the codebase, adding Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utill.rs`:
- Around line 1153-1158: The module-level documentation for the cbor module is
currently outside the module; move that comment block into the cbor module and
convert the leading triple-slash comments to inner-line doc comments (use //!).
Locate the cbor module declaration (mod cbor { ... } or the module's file) and
place the documentation immediately inside the module body (or at the top of the
module file) as //! comments so it becomes true module-level documentation.
In `@src/watch_tower/watcher_error.rs`:
- Line 23: The enum variant SerdeCbor in watcher_error.rs is backend-specific;
rename it to a backend-agnostic name like CborError (wrapping the same
crate::utill::cbor::Error) and update all references. Replace occurrences of
SerdeCbor with CborError in the enum definition and adjust any pattern matches,
From/Into impls, Display/Debug/error conversions, and tests that construct or
match that variant so compilation and behavior remain unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
Cargo.tomlsrc/bin/maker-cli.rssrc/error.rssrc/maker/error.rssrc/maker/rpc/server.rssrc/maker/server.rssrc/maker/server2.rssrc/security.rssrc/taker/api2.rssrc/taker/error.rssrc/taker/offers.rssrc/taker/routines.rssrc/utill.rssrc/wallet/error.rssrc/wallet/ffi.rssrc/wallet/storage.rssrc/watch_tower/registry_storage.rssrc/watch_tower/watcher_error.rs
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 (1)
src/security.rs (1)
270-287:⚠️ Potential issue | 🟠 Major
decrypt_structreturnsResultbut panics on decryption failure.Line 283 uses
.expect(...), which crashes on wrong passphrase instead of returning an error. The return type at line 270 only represents CBOR errors, not AES-GCM decryption failures. This mirrors the asymmetry withencrypt_struct(line 236), which properly returnsEncryptErrorfor both encryption and serialization failures.Suggested fix
+#[derive(Debug)] +pub enum DecryptError { + Decryption, + Deserialization(crate::utill::cbor::Error), +} + +impl From<aes_gcm::Error> for DecryptError { + fn from(_: aes_gcm::Error) -> Self { + DecryptError::Decryption + } +} + +impl From<crate::utill::cbor::Error> for DecryptError { + fn from(err: crate::utill::cbor::Error) -> Self { + DecryptError::Deserialization(err) + } +} + pub fn decrypt_struct<T: DeserializeOwned>( encrypted_struct: EncryptedData, enc_material: &KeyMaterial, -) -> Result<T, crate::utill::cbor::Error> { +) -> Result<T, DecryptError> { @@ - let plaintext_cbor = cipher - .decrypt(&nonce, encrypted_struct.encrypted_payload.as_ref()) - .expect("Error decrypting wallet, wrong passphrase?"); + let plaintext_cbor = cipher + .decrypt(&nonce, encrypted_struct.encrypted_payload.as_ref()) + .map_err(DecryptError::from)?; @@ - utill::deserialize_from_cbor::<T>(plaintext_cbor) + utill::deserialize_from_cbor::<T>(plaintext_cbor).map_err(DecryptError::from) }
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utill.rs`:
- Around line 1195-1208: The mapping of serialization errors in to_writer
currently uses Debug formatting which can lose structured context; change the
map_err call in to_writer so it preserves a more useful error message (e.g., use
Display "{}" formatting of the error instead of "{:?}" or extend the Error enum
to include a variant for writer/serialization errors) so that errors from
minicbor_serde::Serializer and the WriteWrapper adapter are reported with
clearer text or their original error type (reference symbols: to_writer,
WriteWrapper, minicbor_serde::Serializer, Error::Custom).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
Cargo.tomlsrc/bin/maker-cli.rssrc/error.rssrc/maker/error.rssrc/maker/rpc/server.rssrc/maker/server.rssrc/maker/server2.rssrc/security.rssrc/taker/api2.rssrc/taker/error.rssrc/taker/offers.rssrc/taker/routines.rssrc/utill.rssrc/wallet/error.rssrc/wallet/ffi.rssrc/wallet/storage.rssrc/watch_tower/registry_storage.rssrc/watch_tower/watcher_error.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #784 +/- ##
===========================================
+ Coverage 68.87% 80.97% +12.10%
===========================================
Files 35 52 +17
Lines 4932 15063 +10131
===========================================
+ Hits 3397 12198 +8801
- Misses 1535 2865 +1330 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| assert_eq!(returned_pubkey.to_string(), tweaked_pubkey.to_string()); | ||
| } | ||
| } | ||
| pub mod cbor { |
There was a problem hiding this comment.
What is the purpose of these wrappers? from_slice and to_vec are already in minicbor_serde
https://docs.rs/minicbor-serde/0.6.2/minicbor_serde/#functions
There was a problem hiding this comment.
@hulxv Thanks for review, I have added the wrappers to isolate the serialization backend and prevent direct dependency on minicbor_serde across the codebase. This makes future backend swaps less invasive and centralizes error handling.
There was a problem hiding this comment.
Happy to adjust based on feedback
There was a problem hiding this comment.
That makes sense. Will it work if I simplify this by removing the wrappers around to_vec and from_slice by using minicbor_serde directly, keeping only the necessary to_writer adapter?
There was a problem hiding this comment.
Nope we don't need this wrapper. I don't see its doing anything that we can't call inline.
This should not be a module.
Also as per minicbor-serde docs, we should be using minicbor-derive to get a more compact message format.
In contrast to minicbor-derive, this serde-based implementation makes no attempt to be particularly clever with regards to forward and backward compatibility, nor does it use integers instead of strings for struct field names or enum constructors. If those features are important, consider using minicbor-derive instead.
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)
src/utill.rs (1)
728-741:⚠️ Potential issue | 🔴 CriticalTrailing-data recovery loop can spin forever on irrecoverable input.
Line [735] starts an unbounded loop, and Line [736] keeps calling
pop()even after the buffer is empty. If decode never succeeds, this becomes non-terminating.🔧 Proposed fix
Err(e) => { let err_string = format!("{e:?}"); if err_string.contains("code: TrailingData") { // Defensive error handling - monitor logs to confirm wallet files stay clean. log::info!("Wallet file has trailing data, trying to restore"); - loop { - reader.pop(); - match crate::utill::cbor::from_slice::<T>(&reader) { - Ok(store) => break Ok(store), - Err(_) => continue, - } - } + while !reader.is_empty() { + reader.pop(); + if let Ok(store) = crate::utill::cbor::from_slice::<T>(&reader) { + return Ok(store); + } + } + Err(e) } else { Err(e) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utill.rs` around lines 728 - 741, The recovery loop for TrailingData can spin forever because it repeatedly calls reader.pop() without checking emptiness; update the block handling crate::utill::cbor::from_slice::<T>(&reader) so that inside the loop you first check whether reader is empty (or whether pop returned Some) and if so return the original Err(e) (or a new descriptive error) instead of continuing, otherwise pop() and retry decoding; ensure you preserve the original error context when bailing and reference the reader.pop() call and the cbor::from_slice::<T> retry logic in your fix.src/security.rs (1)
270-287:⚠️ Potential issue | 🔴 CriticalStop panicking in
decrypt_struct; return decrypt failures as errors.Line [283] uses
expect(...), so a wrong passphrase/corrupted payload can crash the process even though the function returnsResult. Also, Line [270] only exposes CBOR errors, so AES-GCM failures are not representable.🔧 Proposed fix
+#[derive(Debug)] +pub enum DecryptError { + Decryption, + Deserialization(crate::utill::cbor::Error), +} + pub fn decrypt_struct<T: DeserializeOwned>( encrypted_struct: EncryptedData, enc_material: &KeyMaterial, -) -> Result<T, crate::utill::cbor::Error> { +) -> Result<T, DecryptError> { @@ let plaintext_cbor = cipher .decrypt(&nonce, encrypted_struct.encrypted_payload.as_ref()) - .expect("Error decrypting wallet, wrong passphrase?"); + .map_err(|_| DecryptError::Decryption)?; @@ - utill::deserialize_from_cbor::<T>(plaintext_cbor) + utill::deserialize_from_cbor::<T>(plaintext_cbor) + .map_err(DecryptError::Deserialization) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/security.rs` around lines 270 - 287, decrypt_struct currently panics on AES-GCM failures via expect; change it to return decryption errors instead of panicking by updating decrypt_struct's return type to a combined error (e.g., a new SecurityError enum or an existing error type) that can represent both aes_gcm::Error and crate::utill::cbor::Error, replace the expect call on cipher.decrypt(...) with a match or map_err to convert aes_gcm::Error into that combined error, then propagate that error to the caller before calling utill::deserialize_from_cbor::<T>(...), ensuring encrypted_struct.encrypted_payload, cipher.decrypt and utill::deserialize_from_cbor are referenced when implementing the mapping or From conversions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utill.rs`:
- Around line 697-698: Replace the runtime panics caused by assert_eq!(hostname,
*hostname_data) and assert_eq!(private_key, *private_key_data) with a typed
error return (TorError) so production won't abort when persisted metadata and
live Tor response diverge; inside the function that compares hostname,
hostname_data, private_key, and private_key_data, perform equality checks and if
they differ return an appropriate TorError variant (e.g.,
TorError::MetadataMismatch or TorError::InvalidMetadata) including contextual
info (which field mismatched) instead of calling assert_eq!, and update the
function signature to propagate the TorError (Result<..., TorError>) where
necessary.
---
Outside diff comments:
In `@src/security.rs`:
- Around line 270-287: decrypt_struct currently panics on AES-GCM failures via
expect; change it to return decryption errors instead of panicking by updating
decrypt_struct's return type to a combined error (e.g., a new SecurityError enum
or an existing error type) that can represent both aes_gcm::Error and
crate::utill::cbor::Error, replace the expect call on cipher.decrypt(...) with a
match or map_err to convert aes_gcm::Error into that combined error, then
propagate that error to the caller before calling
utill::deserialize_from_cbor::<T>(...), ensuring
encrypted_struct.encrypted_payload, cipher.decrypt and
utill::deserialize_from_cbor are referenced when implementing the mapping or
From conversions.
In `@src/utill.rs`:
- Around line 728-741: The recovery loop for TrailingData can spin forever
because it repeatedly calls reader.pop() without checking emptiness; update the
block handling crate::utill::cbor::from_slice::<T>(&reader) so that inside the
loop you first check whether reader is empty (or whether pop returned Some) and
if so return the original Err(e) (or a new descriptive error) instead of
continuing, otherwise pop() and retry decoding; ensure you preserve the original
error context when bailing and reference the reader.pop() call and the
cbor::from_slice::<T> retry logic in your fix.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
Cargo.tomlsrc/bin/maker-cli.rssrc/error.rssrc/maker/error.rssrc/maker/rpc/server.rssrc/maker/server.rssrc/maker/server2.rssrc/security.rssrc/taker/api2.rssrc/taker/error.rssrc/taker/offers.rssrc/taker/routines.rssrc/utill.rssrc/wallet/error.rssrc/wallet/ffi.rssrc/wallet/storage.rssrc/watch_tower/registry_storage.rssrc/watch_tower/watcher_error.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 39-40: Confirm whether the project currently uses serde_cbor's
packed/integer-key encoding before replacing it with minicbor-serde: search for
usages of serde_cbor (e.g., serde_cbor::to_vec, serde_cbor::from_slice,
serde_cbor::ser::Serializer with packed/config options) and for all
serialization/deserialization codepaths that handle wallet files and network
messages; if any code paths use packed/integer keys, revert the dependency
change or add a migration/version field and conversion logic for wallet files
and a version/negotiation or compatibility layer for network messages so that
minicbor/minicbor-serde (which assumes string-key encoding for Serde-derived
types) can interoperate; otherwise, proceed with swapping to minicbor and
minicbor-serde.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
Cargo.tomlsrc/bin/maker-cli.rssrc/error.rssrc/maker/error.rssrc/maker/rpc/server.rssrc/maker/server.rssrc/maker/server2.rssrc/security.rssrc/taker/api2.rssrc/taker/error.rssrc/taker/offers.rssrc/taker/routines.rssrc/utill.rssrc/wallet/error.rssrc/wallet/ffi.rssrc/wallet/storage.rssrc/watch_tower/registry_storage.rssrc/watch_tower/watcher_error.rs
|
@coderrabit
I have thoroughly verified the codebase and found that coinswap did not use any packed or integer-key representations. |
| assert_eq!(returned_pubkey.to_string(), tweaked_pubkey.to_string()); | ||
| } | ||
| } | ||
| pub mod cbor { |
There was a problem hiding this comment.
Nope we don't need this wrapper. I don't see its doing anything that we can't call inline.
This should not be a module.
Also as per minicbor-serde docs, we should be using minicbor-derive to get a more compact message format.
In contrast to minicbor-derive, this serde-based implementation makes no attempt to be particularly clever with regards to forward and backward compatibility, nor does it use integers instead of strings for struct field names or enum constructors. If those features are important, consider using minicbor-derive instead.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/watch_tower/registry_storage.rs (1)
83-95:⚠️ Potential issue | 🔴 CriticalDo not swallow container decode failures in
RegistryData::decode.Current
if let Ok(Some(...))patterns discard decode errors and can turn invalid/corrupt payloads into empty in-memory state. That bypasses the intended error fallback path inFileRegistry::loadand risks silent state loss.💡 Suggested fix
- if let Ok(Some(n)) = d.map() { - for _ in 0..n { - let k = decode_outpoint(d, &mut ())?; - let v: WatchRequest = serde_json::from_slice(d.bytes()?) - .map_err(|_| minicbor::decode::Error::message("invalid wrap"))?; - watches.insert(k, v); - } - } + let n = d + .map()? + .ok_or_else(|| minicbor::decode::Error::message("indefinite map not supported"))?; + for _ in 0..n { + let k = decode_outpoint(d, &mut ())?; + let v: WatchRequest = serde_json::from_slice(d.bytes()?) + .map_err(|_| minicbor::decode::Error::message("invalid wrap"))?; + watches.insert(k, v); + } let mut fidelity = HashSet::new(); - if let Ok(Some(n)) = d.array() { - for _ in 0..n { - let f: Fidelity = serde_json::from_slice(d.bytes()?) - .map_err(|_| minicbor::decode::Error::message("invalid wrap"))?; - fidelity.insert(f); - } - } + let n = d + .array()? + .ok_or_else(|| minicbor::decode::Error::message("indefinite array not supported"))?; + for _ in 0..n { + let f: Fidelity = serde_json::from_slice(d.bytes()?) + .map_err(|_| minicbor::decode::Error::message("invalid wrap"))?; + fidelity.insert(f); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/watch_tower/registry_storage.rs` around lines 83 - 95, RegistryData::decode currently uses if let Ok(Some(...)) on d.map() and d.array(), which silently ignores Err cases and treats corrupt payloads as empty; update RegistryData::decode to explicitly propagate decode errors instead of swallowing them by matching the Result from d.map() and d.array() (or using ? where applicable), so any Err returned by the decoder is returned from RegistryData::decode (preserving intent in FileRegistry::load). Ensure decoding of entries (calls to decode_outpoint, serde_json::from_slice for WatchRequest and Fidelity, and insertion into watches and fidelity) only runs when the container count is Ok(Some(n)); return an appropriate minicbor::decode::Error for Err cases rather than falling through.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bin/maker-cli.rs`:
- Around line 141-142: Replace the panic-causing expect when decoding RPC
responses so decode errors are returned as a MakerError instead of aborting:
change the decode call in maker-cli.rs (where response: RpcMsgResp is created)
to propagate errors (use map_err or ? to convert minicbor::decode's error into
an appropriate MakerError variant like MakerError::RpcDecode or add that variant
if missing) and return Result from the surrounding function so callers receive
the error path rather than panicking.
In `@src/maker/rpc/messages.rs`:
- Around line 209-222: The encoder encode_path_buf currently uses
x.to_str().unwrap_or("") which silently drops non-UTF8 paths; change
encode_path_buf to fail explicitly when to_str() is None by returning an
appropriate minicbor encode error instead of encoding an empty string (so
callers get a serialization error). Update encode_path_buf to check x.to_str(),
and on None return Err(minicbor::encode::Error::custom(...) or similar) with a
clear message; leave decode_path_buf unchanged (it can continue decoding a
String into PathBuf) so round-trip will surface non-UTF8 path errors during
encoding.
In `@src/protocol/messages2.rs`:
- Around line 338-349: The function decode_opt_public_key currently swallows
parsing failures by mapping FromStr::from_str(&s) errors to Ok(None); change it
so parse failures return Err(minicbor::decode::Error) instead of Ok(None): after
decoding the string in decode_opt_public_key, attempt to parse with
std::str::FromStr::from_str(&s) and map any parse error into a
minicbor::decode::Error (with a descriptive message like "invalid opt public
key"), then return that Err; keep the existing handling for Null values and only
return Ok(Some(pubkey)) on successful parse.
In `@src/wallet/swapcoin.rs`:
- Around line 1165-1174: The optional decoders (decode_opt_secret_key and
decode_opt_signature) currently swallow parse failures by calling .ok(), turning
malformed non-null inputs into None; change them to mirror decode_secret_key by
returning an Err on invalid data: when the type is not Null, read the bytes with
d.bytes()? and attempt to create the item with SecretKey::from_slice(...) (or
Signature::from_slice(...)) and convert any failure into a
minicbor::decode::Error (propagate with ? or map the failure to an appropriate
decode error) instead of returning Ok(None), so malformed payloads fail decoding
rather than silently coercing to None.
In `@src/watch_tower/registry_storage.rs`:
- Around line 61-67: The two uses of serde_json::to_vec(...).unwrap() in the
registry serialization (the v_bytes and f_bytes conversions inside the loops in
registry_storage.rs) must not panic; instead map the serde_json error into and
return an encode error so persistence can fail gracefully. Replace the unwraps
with error-aware conversions (e.g., serde_json::to_vec(v).map_err(|err| /*
convert into the same error type returned by e.bytes / the function */ )?) and
propagate that error out of the serialization function so e.bytes is only called
on a successful byte vector; do the same for the f_bytes path.
---
Duplicate comments:
In `@src/watch_tower/registry_storage.rs`:
- Around line 83-95: RegistryData::decode currently uses if let Ok(Some(...)) on
d.map() and d.array(), which silently ignores Err cases and treats corrupt
payloads as empty; update RegistryData::decode to explicitly propagate decode
errors instead of swallowing them by matching the Result from d.map() and
d.array() (or using ? where applicable), so any Err returned by the decoder is
returned from RegistryData::decode (preserving intent in FileRegistry::load).
Ensure decoding of entries (calls to decode_outpoint, serde_json::from_slice for
WatchRequest and Fidelity, and insertion into watches and fidelity) only runs
when the container count is Ok(Some(n)); return an appropriate
minicbor::decode::Error for Err cases rather than falling through.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (24)
Cargo.tomlsrc/bin/maker-cli.rssrc/error.rssrc/maker/error.rssrc/maker/rpc/messages.rssrc/maker/rpc/server.rssrc/maker/server.rssrc/maker/server2.rssrc/protocol/messages.rssrc/protocol/messages2.rssrc/security.rssrc/taker/api2.rssrc/taker/error.rssrc/taker/offers.rssrc/taker/routines.rssrc/utill.rssrc/wallet/backup.rssrc/wallet/error.rssrc/wallet/ffi.rssrc/wallet/fidelity.rssrc/wallet/storage.rssrc/wallet/swapcoin.rssrc/watch_tower/registry_storage.rssrc/watch_tower/watcher_error.rs
|
@mojoX911 @hulxv, I have implemented migration using |
|
Let me know if this approach works, happy to adjust on feedback. |
Ferryx349
force-pushed
the
b2
branch
3 times, most recently
from
e31d34e to
0a38af3
Compare
Signed-off-by: ABHAY PANDEY <pandeyabhay967@gmail.com>
|
While resolving merge conflicts during rebase, by mistake I have removed few varients which resulted in CI. I have fixed and tests are passing locally now. |
|
That's too much for replacing a library. After searching, I found |
|
Got it, that makes sense, using |
3 participants
Fixes- #782
Migrated from
serde_cbortominicbor_serdeto maintain serde-based serialization across the codebase.Included
minicborexplicitly for implementing a customto_writeradapter.Ref.- https://docs.rs/minicbor-serde/latest/minicbor_serde/
Summary by CodeRabbit