Conversation
Adds `buyer_dispute` and `seller_dispute` fields to `RestoredDisputeHelper` struct. Also, adds `initiator` field to the `RestoredDisputesInfo` struct to indicate who initiated the dispute.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughRestoredDisputeHelper gains two booleans ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (3)
src/message.rs (3)
279-280: Consider adding///doc comments for the new boolean fields.The existing fields in this struct lack doc comments too, but since these booleans carry specific semantics (per the PR: used to derive
initiatorin Rust after decryption), brief docs would help future readers understand their purpose. As per coding guidelines: "Document public APIs with /// comments."Also, there's no test coverage for
RestoredDisputeHelperwith these new fields — a serialization round-trip test would be valuable given thesqlxfeature gate.📝 Suggested doc comments
+ /// Whether the buyer initiated the dispute (used to derive `initiator` after decryption) pub buyer_dispute: bool, + /// Whether the seller initiated the dispute (used to derive `initiator` after decryption) pub seller_dispute: bool,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/message.rs` around lines 279 - 280, Add /// doc comments to the new boolean fields buyer_dispute and seller_dispute in the RestoredDisputeHelper struct describing their semantics (e.g., which party they indicate and that they are used to derive initiator after decryption), and update the public API docs accordingly; then add a serialization round-trip test for RestoredDisputeHelper that constructs an instance with buyer_dispute/seller_dispute set, serializes and deserializes (or uses sqlx row round-trip under the sqlx feature gate) and asserts the booleans are preserved to cover the new fields.
761-767: Test only coversinitiator: Some("buyer")— add a case forNone.The
initiatorfield can beNone(unknown initiator). Adding a second dispute entry withinitiator: Noneto this test vector would verify that theOptionserialization/deserialization handles the absent case correctly.📝 Suggested addition
let restored_disputes = vec![crate::message::RestoredDisputesInfo { dispute_id: uuid!("508e1272-d5f4-47e6-bd97-3504baea9c25"), order_id: uuid!("308e1272-d5f4-47e6-bd97-3504baea9c23"), trade_index: 1, status: "initiated".to_string(), initiator: Some("buyer".to_string()), -}]; +}, +crate::message::RestoredDisputesInfo { + dispute_id: uuid!("608e1272-d5f4-47e6-bd97-3504baea9c26"), + order_id: uuid!("408e1272-d5f4-47e6-bd97-3504baea9c24"), + trade_index: 2, + status: "initiated".to_string(), + initiator: None, +}];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/message.rs` around lines 761 - 767, The test vector restored_disputes currently only includes a RestoredDisputesInfo with initiator: Some("buyer"); add a second RestoredDisputesInfo entry to restored_disputes with a different dispute_id/order_id/trade_index and initiator: None to ensure Option serialization/deserialization is exercised for the absent case; update any assertions to expect two entries (including the one with initiator == None) and reuse the same RestoredDisputesInfo struct name to locate the code block.
307-308: Free-formStringfor a field with a fixed set of values — consider a typed enum.The
initiatorfield is documented as"buyer","seller", ornull, which maps naturally to anOption<DisputeInitiator>enum. A string allows arbitrary values (e.g.,"Buyer","BUYER", typos) that would silently pass deserialization. If backward compatibility with existing JSON or SQL output requires a string, a#[serde(rename_all = ...)]enum can still produce the same wire format.Not blocking, but worth considering for type safety — especially since this is a public API consumed by clients.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/message.rs` around lines 307 - 308, Replace the free-form initiator: Option<String> with a typed enum to enforce allowed values: define an enum DisputeInitiator (e.g., enum DisputeInitiator { Buyer, Seller }) derived Serialize/Deserialize and annotate with #[serde(rename_all = "lowercase")] so the wire format remains "buyer"/"seller", then change the field to pub initiator: Option<DisputeInitiator> in the same struct; update any serialization/deserialization call sites/tests that assume a raw string accordingly. Ensure the enum derives Debug/Clone/PartialEq (and any other traits used in this crate) to keep parity with the previous String usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/message.rs`:
- Around line 279-280: Add /// doc comments to the new boolean fields
buyer_dispute and seller_dispute in the RestoredDisputeHelper struct describing
their semantics (e.g., which party they indicate and that they are used to
derive initiator after decryption), and update the public API docs accordingly;
then add a serialization round-trip test for RestoredDisputeHelper that
constructs an instance with buyer_dispute/seller_dispute set, serializes and
deserializes (or uses sqlx row round-trip under the sqlx feature gate) and
asserts the booleans are preserved to cover the new fields.
- Around line 761-767: The test vector restored_disputes currently only includes
a RestoredDisputesInfo with initiator: Some("buyer"); add a second
RestoredDisputesInfo entry to restored_disputes with a different
dispute_id/order_id/trade_index and initiator: None to ensure Option
serialization/deserialization is exercised for the absent case; update any
assertions to expect two entries (including the one with initiator == None) and
reuse the same RestoredDisputesInfo struct name to locate the code block.
- Around line 307-308: Replace the free-form initiator: Option<String> with a
typed enum to enforce allowed values: define an enum DisputeInitiator (e.g.,
enum DisputeInitiator { Buyer, Seller }) derived Serialize/Deserialize and
annotate with #[serde(rename_all = "lowercase")] so the wire format remains
"buyer"/"seller", then change the field to pub initiator:
Option<DisputeInitiator> in the same struct; update any
serialization/deserialization call sites/tests that assume a raw string
accordingly. Ensure the enum derives Debug/Clone/PartialEq (and any other traits
used in this crate) to keep parity with the previous String usage.
|
uTACK for me! |
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
please address rabbit nitpicks
Updates the `RestoredDisputesInfo` model to use an enum (`DisputeInitiator`) instead of a string for representing who initiated a dispute. This provides type safety and better represents the possible states. Also adds test coverage for serialization and deserialization.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/message.rs`:
- Line 785: The tests use the wrong status string: update the status field in
the test fixtures inside test_restore_session_message and
test_restored_dispute_helper_serialization_roundtrip to use "in-progress"
(hyphen) instead of "in_progress" (underscore) so they match the canonical
status parsed by the Dispute/Status parsing logic; locate the two occurrences of
status: "in_progress".to_string() and replace them with status:
"in-progress".to_string().
- Around line 788-794: The third RestoredDisputesInfo test entry duplicates the
first entry's dispute_id (and order_id) due to a copy/paste; update the third
RestoredDisputesInfo struct instance so its dispute_id (and order_id if
appropriate) use distinct UUIDs (for example
"708e1272-d5f4-47e6-bd97-3504baea9c27") instead of reusing
uuid!("508e1272-d5f4-47e6-bd97-3504baea9c25"), ensuring each
RestoredDisputesInfo has a unique dispute_id (and unique order_id when
required).
Adds SQLx compatibility to the `DisputeInitiator` enum, enabling database storage. The changes involve adding the `sqlx::Type` derive and specifying the `type_name` and `rename_all` attributes for proper mapping in SQLx.
Updates the restored disputes info to ensure data integrity and consistency in dispute-related operations. Corrects the dispute status from "in_progress" to "in-progress" and updates dispute IDs, order IDs, and trade indices for better accuracy.
3 participants
RestoredDisputesInfowas missing theinitiatorfield. The struct is used in the restore-session response sent to clients.Proposed changes:
initiator: Option<String>toRestoredDisputesInfobuyer_dispute: boolandseller_dispute: booltoRestoredDisputeHelperso the encrypted-DB path in mostro can carry those values into Rust and derive the initiator after decryptionCloses #129
Summary by CodeRabbit