Make cross regions more typed

[SozinM]

📝 Summary

Option is bad

💡 Motivation and Context

---

✅ I have completed the following steps:

• Run make lint
• Run make test
• Added tests (if applicable)

[claude]

Claude finished @SozinM's task in 1m 32s —— View job

---

Review

• Read the PR diff
• Check for missed call sites
• Check serde compatibility
• Post review

Findings:

1. Compile error — missed call site. crates/rbuilder/src/backtest/store.rs:747 still initializes disable_cross_region_sharing: None after the field's type change to bool. The workspace will not compile until this is updated to false. Also worth running rg "disable_cross_region_sharing" and cargo check --workspace before merging — make lint/make test are unchecked in the PR description. Fix this →

2. Wire-format compatibility (serialize.rs:158). The previous Option<bool> accepted "disable_cross_region_sharing": null from external producers; the new bool will reject null at deserialization time. Omitting the field still works thanks to #[serde(default)], but explicit null is no longer tolerated. Confirm no upstream client emits null here, or keep RawBundleMetadata's wire field as Option<bool> and convert to bool at the boundary.

3. Subtle V1 validation behavior change (serialize.rs:241). The check went from self.disable_cross_region_sharing.is_some() to self.disable_cross_region_sharing. Previously Some(false) would have been rejected for V1 bundles ("field not supported"); now false is silently accepted. This is probably the intended/more correct semantics, but flagging in case callers relied on the strict is_some check.

Inline comments left on points 2 and 3. Point 1 couldn't be left inline because store.rs isn't part of the diff.
· msozin/make-cross-region-more-typed

[claude]

Subtle behavior change: previously validate_fields errored on Some(false) (because is_some() was checked), now it only errors when the flag is actually true. This is arguably more correct, but if any caller relied on V1 rejecting bundles that explicitly serialized "disable_cross_region_sharing": false, that rejection is gone. Worth confirming this is intended.

[claude]

Wire-format compatibility note: the old Option<bool> accepted "disable_cross_region_sharing": null from clients (deserialized to None). With plain bool, an explicit null will now fail deserialization. Omitting the field still works because of #[serde(default)], but if any external producer sends explicit null, it will break. If you want to keep tolerating that, use #[serde(default, deserialize_with = "...")] to coerce null to false, or keep the Option<bool> only on RawBundleMetadata and convert at the boundary.

[claude]

Claude finished @SozinM's task in 1m 1s —— View job

---

Re-review after fmt commit

The latest commit only reformats lib.rs. The issues from the previous review still stand:

• Re-checked PR diff
• Re-checked call sites
• Verified inline-comment points still apply
• Posted update

Findings:

1. Still broken — compile error in crates/rbuilder/src/backtest/store.rs:747. The test initializer still has disable_cross_region_sharing: None, while the field type is now bool. This file isn't part of the PR diff, so the fmt commit didn't touch it. The workspace will not compile until this is changed to false. Fix this →

2. Wire-format compatibility (serialize.rs:158) — unchanged. The previous Option<bool> accepted "disable_cross_region_sharing": null; the new plain bool will reject null at deserialization. Omitting the field is still fine via #[serde(default)]. Worth confirming no upstream client emits explicit null, or convert Option<bool> -> bool at the wire boundary.

3. V1 validation semantics (serialize.rs:241) — unchanged. The check went from is_some() to the bool itself. Previously Some(false) would be rejected for V1 bundles; now false is silently accepted. Probably intended, but flagging in case any caller relied on the strict is_some check.

Recommend: fix point 1 and run make lint / make test (still unchecked in the PR description).
· msozin/make-cross-region-more-typed

[claude]

Claude finished @SozinM's task in 54s —— View job

---

Re-review after Fix missed None commit (a501b8b)

• Re-checked PR diff
• Verified all disable_cross_region_sharing call sites
• Re-checked inline-comment points

Findings:

1. ✅ Compile error is fixed. Commit a501b8b updates both crates/rbuilder/src/backtest/store.rs:747 and crates/rbuilder/src/backtest/build_block/synthetic_orders.rs:101 to disable_cross_region_sharing: false. rg "disable_cross_region_sharing" now shows all sites consistently using bool. The removed set_disable_cross_region_sharing setter has no callers, so dropping it is safe.

2. Wire-format compatibility (serialize.rs:158) — still applies. Old Option<bool> accepted "disable_cross_region_sharing": null; new plain bool will reject null at deserialization. Omitting the field is still fine via #[serde(default)]. Confirm no upstream client emits explicit null, otherwise convert at the wire boundary.

3. V1 validation semantics (serialize.rs:241) — still applies. Check went from is_some() to the bool itself, so Some(false) (previously rejected for V1) is now silently accepted. Probably intentional — flagging in case any caller relied on the strict is_some check.

Recommend: run make lint / make test (still unchecked in the PR description) before merge.
· msozin/make-cross-region-more-typed