feat: Handle account shrinking/expansion in merge_diff_copy

[snawaz]

The merge_diff_copy implemented by the previous PR #113 works for accounts which do not shink/expand on ER.

With this PR, merge_diff_copy() handles account shrinking/expansion.

Summary by CodeRabbit

• Bug Fixes

  • Enhanced diff merging validation to better detect and handle edge cases with improved error reporting

• Tests

  • Expanded test coverage for diff operations including edge-case scenarios

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[snawaz]

• feat: Handle account shrinking/expansion in merge_diff_copy #116 👈 (View in Graphite)
• feat: Add CommitDiffFromBuffer ix #118
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/diff/algorithm.rs (1)

500-543: Good expansion test with documented future optimization.

The test validates account expansion correctly. The TODO comments at lines 524-525 and 535-536 clearly document that currently expanded zero bytes are included in the diff, and that future optimization could skip them to let unwritten contain those bytes instead.

Would you like me to open an issue to track the optimization mentioned in the TODO comments (not including zero bytes in expansion diffs)?

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1bcaf4d and f860453.

📒 Files selected for processing (2)

• src/diff/algorithm.rs (3 hunks)
• src/error.rs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/diff/algorithm.rs (1)

src/diff/types.rs (2)

• changed_len (121-123)
• try_new (45-104)

⏰ 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: lint

🔇 Additional comments (8)

src/diff/algorithm.rs (8)

235-260: Clear documentation for the updated contract.

The documentation thoroughly explains the preconditions, assumptions, and return semantics. The explicit note that zero-initialization is assumed but not validated for performance is reasonable for a Solana program context where freshly allocated account data is zero-initialized.

---

269-275: LGTM - bounds validation for original slice access.

The check at line 270 correctly prevents out-of-bounds access on original at line 274. This handles the case where a corrupted diff might reference a start position beyond the original data length.

---

283-337: Well-structured handling of all post-diff scenarios.

The three-scenario logic correctly handles:

1. Equal: Destination fully written—nothing more to do.
2. Less: Unwritten bytes filled from original (shrink) or left zero-initialized (expand).
3. Greater: Logically impossible state caught with error.

The nested conditions in the Less branch correctly distinguish between shrink vs. expand cases and partial vs. full zero-initialization.

---

339-341: LGTM - clean return of unwritten slice.

Using split_at_mut to return the trailing unwritten portion allows callers to validate zero-initialization or handle expanded regions as needed.

---

380-430: LGTM - useful test helper for constructing expected diffs.

The helper correctly builds the expected binary diff format, and the comment at line 382 clearly documents the constraint that additional_changes must apply after index 78.

---

464-498: Good test coverage for the shrink scenario.

The test correctly validates that merge_diff_copy handles account shrinking, with CHANGED_LEN = 80 chosen to be just past the last diff index (78). This ensures the truncated bytes from original are properly discarded.

---

545-640: Excellent randomized test coverage.

The large random data test provides strong coverage with:

• Random account sizes (2-10 MB)
• Non-overlapping patches at random positions
• Seeded RNG for reproducibility (seed printed on failure)
• Verification of both apply_diff_in_place and merge_diff_copy

---

277-278: No action needed. DiffSet validation prevents the potential bounds issue.

The validation in DiffSet::diff_segment_at() (types.rs lines 160-171) comprehensively checks that segment offsets are valid before the iterator returns them. Specifically, line 171 validates range.end > self.changed_len() and returns Err if violated. Since merge_diff_copy verifies the precondition destination.len() == diffset.changed_len() at entry (line 261), the slice access at line 277 is guaranteed to be in bounds.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Typo in comment.

Line 333 contains "destinatiare" which should be "destination".

🔎 Proposed fix

         Ordering::Greater => {
-            // It is an impossible scenario. Even if the diff is corrupt, or the lengths of destinatiare are same
+            // It is an impossible scenario. Even if the diff is corrupt, or the lengths of destination are same
             // or different, we'll not encounter this case. It only implies logic error.
             return Err(DlpError::InfallibleError.into());
         }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	Ordering::Greater => {
	// It is an impossible scenario. Even if the diff is corrupt, or the lengths of destinatiare are same
	// or different, we'll not encounter this case. It only implies logic error.
	return Err(DlpError::InfallibleError.into());
	}
	Ordering::Greater => {
	// It is an impossible scenario. Even if the diff is corrupt, or the lengths of destination are same
	// or different, we'll not encounter this case. It only implies logic error.
	return Err(DlpError::InfallibleError.into());
	}

🤖 Prompt for AI Agents

In src/diff/algorithm.rs around lines 332 to 336, fix the typo in the comment:
change "destinatiare" to "destination" so the comment reads correctly; update
the single-line comment and keep all other wording unchanged.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Consider renaming InfallibleError for clarity.

The name "InfallibleError" is contradictory—"infallible" typically means "incapable of failing." A clearer name might be InternalLogicError, UnreachableStateError, or InvariantViolation to better convey that this represents a state that should be logically impossible.

🔎 Suggested rename

-    #[error("An infallible error is encountered possibly due to logic error")]
-    InfallibleError = 100,
+    #[error("Internal invariant violation: reached logically impossible state")]
+    InvariantViolation = 100,

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In src/error.rs around lines 84 to 85, rename the enum variant InfallibleError
to a clearer name (e.g., InternalLogicError or InvariantViolation) and update
its display/error string accordingly; then update all references (matches,
constructors, conversions, tests, and documentation) across the codebase to use
the new variant name so compilation and error messages remain consistent.

[GabrielePicco]

LGTM!