Skip to content

fix(builder): clone MigratedClasses in cloneStateDiff - #4032

Merged
infrmtcs merged 1 commit into
mainfrom
dat/builder-clone-migrated-classes
Sep 7, 2026
Merged

fix(builder): clone MigratedClasses in cloneStateDiff#4032
infrmtcs merged 1 commit into
mainfrom
dat/builder-clone-migrated-classes

Conversation

@infrmtcs-agent

@infrmtcs-agent infrmtcs-agent Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

User description

Summary

  • cloneStateDiff was missing a clone of MigratedClasses, so the cloned state diff shared the same underlying slice/backing array as the original
  • Added the clone and covered it with a test

PR Type

Bug fix, Tests


Description

  • Clone MigratedClasses to avoid shared state

  • Add test covering full state diff fields

  • Prevent clone mutations affecting original state


File Walkthrough

Relevant files
Bug fix
state.go
Clone MigratedClasses in StateDiff clone                                 

builder/state.go

  • Adds MigratedClasses: maps.Clone(stateDiff.MigratedClasses) to
    cloneStateDiff
+1/-0     
Tests
state_test.go
Add state diff clone coverage test                                             

builder/state_test.go

  • Adds TestCloneCopiesEveryStateDiffField validating all StateDiff
    fields clone
  • Uses reflection fixture and mutation check to catch shared backing
    data
+55/-0   

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @infrmtcs-agent[bot]'s task in 2m 53s —— View job


Reviewing PR #4032

  • Gather context (diff, changed files, repo guidelines)
  • Review builder/state.go change
  • Review builder/state_test.go new test
  • Post findings / summary

Summary

The PR is correct and clean — no issues found, no inline comments needed.

  • MigratedClasses (map[felt.SierraClassHash]felt.CasmClassHash) is a value-typed map, so maps.Clone is the right and sufficient fix, consistent with how the other map fields (Nonces, DeployedContracts, DeclaredV1Classes, ReplacedClasses) are already cloned.
  • The new test (TestCloneCopiesEveryStateDiffField) is well designed: it uses reflect to assert every StateDiff field is populated in the fixture (so a future field addition can't silently go unclonned/untested), then calls Merge on the original and asserts the clone is unaffected — this correctly exercises map aliasing for MigratedClasses (and all other map fields) via Merge's maps.Copy into the receiver's maps, and slice aliasing for DeclaredV0Classes.
  • Verified go vet ./builder/... passes (type-checks the new test file cleanly). go test itself wasn't permitted to run in this sandbox, but the vet pass plus manual trace of Merge/cloneStateDiff confirms the fix and test are sound.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🏅 Score: 95
🧪 PR contains tests
🔒 No security concerns identified
⚡ No major issues detected

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.19%. Comparing base (121146c) to head (35bc8ec).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4032      +/-   ##
==========================================
- Coverage   79.31%   79.19%   -0.13%     
==========================================
  Files         464      464              
  Lines       35793    35788       -5     
==========================================
- Hits        28389    28341      -48     
- Misses       7395     7438      +43     
  Partials        9        9              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@infrmtcs-agent
infrmtcs-agent Bot deployed to Development September 4, 2026 05:03 Active
@infrmtcs
infrmtcs requested a review from rodrodros September 7, 2026 03:16
@infrmtcs
infrmtcs merged commit 58fcf30 into main Sep 7, 2026
30 checks passed
@infrmtcs
infrmtcs deleted the dat/builder-clone-migrated-classes branch September 7, 2026 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants