replace_to post upgrade upgradability validation

[remollemo]

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 77.27273% with 35 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@3d0c557). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...ges/utils/src/components/replaceability/test.cairo	80.55%	21 Missing ⚠️
.../utils/src/components/replaceability/mock_v2.cairo	43.75%	9 Missing ⚠️
...src/components/replaceability/replaceability.cairo	86.95%	3 Missing ⚠️
...s/utils/src/components/replaceability/errors.cairo	33.33%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #164   +/-   ##
=======================================
  Coverage        ?   94.03%           
=======================================
  Files           ?       58           
  Lines           ?     4124           
  Branches        ?        0           
=======================================
  Hits            ?     3878           
  Misses          ?      246           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 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.

[liorgold2]

@liorgold2 made 6 comments.
Reviewable status: 0 of 6 files reviewed, 6 unresolved discussions (waiting on remollemo).

---

packages/utils/src/components/replaceability/replaceability.cairo line 92 at r1 (raw file):

            if (!implementation_data.final) {
                self.invoke_upgradeability_validation(implementation_data.impl_hash);
            }

assert (!implementation_data.final)

Code quote:

            if (!implementation_data.final) {
                self.invoke_upgradeability_validation(implementation_data.impl_hash);
            }

---

packages/utils/src/components/replaceability/replaceability.cairo line 216 at r1 (raw file):

            // `deploy_replaceability_mock` to bridge snforge's zero default.
            self.upgrade_delay.write(0);
            dispatcher.add_new_implementation_unsafe(:implementation_data);

Before this, finish the original upgrade.

---

packages/utils/src/components/replaceability/replaceability.cairo line 218 at r1 (raw file):

            dispatcher.add_new_implementation_unsafe(:implementation_data);
            dispatcher.replace_to(:implementation_data);

Check

get_class_hash_at_syscall(get_contract_address())
                .unwrap_syscall() == impl_hash

---

packages/utils/src/components/replaceability/replaceability.cairo line 258 at r1 (raw file):

                class_hash: current_class_hash,
                function_selector: selector!("validate_upgradeability"),
                calldata: calldata.span(),

Suggestion:

calldata: array![new_class_hash.into()].span(),

---

packages/utils/src/components/replaceability/replaceability.cairo line 262 at r1 (raw file):

            match result {
                Result::Ok(_) => core::panic_with_felt252('VALIDATION_DID_NOT_PANIC'),

Use let panic_data = result.expect_err('VALIDATION_DID_NOT_PANIC').

Code quote:

            match result {
                Result::Ok(_) => core::panic_with_felt252('VALIDATION_DID_NOT_PANIC'),

---

packages/utils/src/components/replaceability/replaceability.cairo line 266 at r1 (raw file):

                    // The runtime appends 'ENTRYPOINT_FAILED' to panic data from a failed entry
                    // point, so match on element 0 rather than the whole array.
                    if panic_data.is_empty()

Try panic_data == array![UPGRADEABILITY_VALIDATION_SUCCESS, 'ENTRYPOINT_FAILED']
If not, check length == 2 (!(lenth == 2 && at(0) == ...))

[remollemo]

@remollemo made 6 comments.
Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on liorgold2 and remollemo).

---

packages/utils/src/components/replaceability/replaceability.cairo line 92 at r1 (raw file):

Previously, liorgold2 wrote…

assert (!implementation_data.final)

Done.

---

packages/utils/src/components/replaceability/replaceability.cairo line 216 at r1 (raw file):

Previously, liorgold2 wrote…

Before this, finish the original upgrade.

Done.

---

packages/utils/src/components/replaceability/replaceability.cairo line 218 at r1 (raw file):

Previously, liorgold2 wrote…

Check

get_class_hash_at_syscall(get_contract_address())
                .unwrap_syscall() == impl_hash

Done.

---

packages/utils/src/components/replaceability/replaceability.cairo line 262 at r1 (raw file):

Previously, liorgold2 wrote…

Use let panic_data = result.expect_err('VALIDATION_DID_NOT_PANIC').

Done.

---

packages/utils/src/components/replaceability/replaceability.cairo line 266 at r1 (raw file):

Previously, liorgold2 wrote…

Try panic_data == array![UPGRADEABILITY_VALIDATION_SUCCESS, 'ENTRYPOINT_FAILED']
If not, check length == 2 (!(lenth == 2 && at(0) == ...))

Done.

---

packages/utils/src/components/replaceability/replaceability.cairo line 258 at r1 (raw file):

                class_hash: current_class_hash,
                function_selector: selector!("validate_upgradeability"),
                calldata: calldata.span(),

not applicable, we pass implementation_data now, which is more complex.

[liorgold2]

@liorgold2 reviewed 3 files and all commit messages, made 12 comments, and resolved 3 discussions.
Reviewable status: 3 of 7 files reviewed, 15 unresolved discussions (waiting on remollemo).

---

packages/utils/src/components/replaceability/errors.cairo line 13 at r2 (raw file):

    EIC_LIB_CALL_FAILED,
    REPLACE_CLASS_HASH_FAILED,
    FAILED_REPLACE_CLASS_HASH_B2A,

What's B2A?

Code quote:

    FAILED_REPLACE_CLASS_HASH_B2A,

---

packages/utils/src/components/replaceability/interface.cairo line 5 at r2 (raw file):

/// Sentinel panic value used by `validate_upgradeability` to signal a successful dry-run.
/// Any other panic from a dry-run is treated as a real failure.
pub(crate) const UPGRADEABILITY_VALIDATION_SUCCESS: felt252 = 'UPGRADEABILITY_OK';

Should it be here or in the impl file?

---

packages/utils/src/components/replaceability/interface.cairo line 46 at r2 (raw file):

    ) -> u64;
    fn add_new_implementation(ref self: TContractState, implementation_data: ImplementationData);
    fn add_new_implementation_unsafe(

Document. E.g., A version of add_new_implementation that skips the future upgradability test.

---

packages/utils/src/components/replaceability/interface.cairo line 54 at r2 (raw file):

    // `add_new_implementation`, never directly.
    // It must live in `IReplaceable` so every contract embedding
    // `ReplaceabilityImpl` exports the selector; otherwise the library_call dispatch fails.

Instruct claude not to use —.
Start the doc with what the function does, only later mention it always panics.

Suggestion:

    // Always panics by design - Should be invoked only via library_call from
    // `add_new_implementation`, never directly.
    // It must live in `IReplaceable` so every contract embedding
    // `ReplaceabilityImpl` exports the selector; otherwise the library_call dispatch fails.

---

packages/utils/src/components/replaceability/interface.cairo line 54 at r2 (raw file):

    // `add_new_implementation`, never directly.
    // It must live in `IReplaceable` so every contract embedding
    // `ReplaceabilityImpl` exports the selector; otherwise the library_call dispatch fails.

I didn't understand these lines. Are they important?

Code quote:

    // It must live in `IReplaceable` so every contract embedding
    // `ReplaceabilityImpl` exports the selector; otherwise the library_call dispatch fails.

---

packages/utils/src/components/replaceability/replaceability.cairo line 76 at r2 (raw file):

        }

        // Schedules a new implementation and validates that it is upgradeable. If the target

Maybe move doc to the trait?

---

packages/utils/src/components/replaceability/replaceability.cairo line 77 at r2 (raw file):

        // Schedules a new implementation and validates that it is upgradeable. If the target
        // cannot itself perform a full upgrade cycle (add + replace, in both directions), the

WDYM?

Code quote:

in both directions

---

packages/utils/src/components/replaceability/replaceability.cairo line 80 at r2 (raw file):

        // entire transaction reverts. Final adds (`implementation_data.final = true`) are
        // rejected with `FINALIZE_IS_UNSAFE` — to finalize, use
        // `add_new_implementation_unsafe` instead.

Replace — everywhere in the PR.

Suggestion:

        // entire transaction reverts. Finalization upgrades (`implementation_data.final = true`) are
        // rejected with `FINALIZE_IS_UNSAFE` - to finalize, use
        // `add_new_implementation_unsafe` instead.

---

packages/utils/src/components/replaceability/replaceability.cairo line 98 at r2 (raw file):

            // validation: step 2's `impl_b_dispatcher.add_new_implementation_unsafe` runs
            // this check on the target class, so a target with broken `upgrade_governor`
            // role wiring fails the dry-run cycle here.

Not sure this comment is helpful (or readable).

Code quote:

            // Authoritative auth check for direct callers. Also load-bearing during
            // validation: step 2's `impl_b_dispatcher.add_new_implementation_unsafe` runs
            // this check on the target class, so a target with broken `upgrade_governor`
            // role wiring fails the dry-run cycle here.

---

packages/utils/src/components/replaceability/replaceability.cairo line 188 at r2 (raw file):

        // failure otherwise. Callers wrap this in a library_call so the panic reverts every
        // side effect: storage writes, emitted events, and the `replace_class_syscall`s in
        // both `replace_to`s below.

This comment is not clear. Let's discuss.

Code quote:

        // Dry-run validation of a full upgrade cycle for the user's `implementation_data`.
        // Always panics — `UPGRADEABILITY_VALIDATION_SUCCESS` on success, or the underlying
        // failure otherwise. Callers wrap this in a library_call so the panic reverts every
        // side effect: storage writes, emitted events, and the `replace_class_syscall`s in
        // both `replace_to`s below.

---

packages/utils/src/components/replaceability/replaceability.cairo line 215 at r2 (raw file):

            let impl_b_hash = implementation_data.impl_hash;

            // Step 1 (A→B): complete the user-planned upgrade on `self`. Zero the delay so

Fix comments.

---

packages/utils/src/components/replaceability/replaceability.cairo line 223 at r2 (raw file):

            self.add_new_implementation_unsafe(implementation_data);
            self.replace_to(implementation_data);

Add sanity check that the current class hash is now impl_b_hash. This makes sure the EIC didn't upgrade us to another class.

[liorgold2]

@liorgold2 reviewed 1 file and resolved 3 discussions.
Reviewable status: 4 of 7 files reviewed, 12 unresolved discussions (waiting on remollemo).

[cursor]

PR Summary

High Risk
High risk because it changes the upgrade scheduling flow to perform a dry-run replace_class roundtrip via library calls and introduces new “unsafe” upgrade paths; mistakes here can block upgrades or incorrectly allow unsafe/finalizing upgrades.

Overview
Adds pre-upgrade validation to ReplaceabilityComponent. add_new_implementation now rejects final upgrades and performs an on-chain dry-run (validate_upgradeability) that simulates A→B and B→A upgrades (including EIC execution) and only schedules the upgrade if the roundtrip succeeds.

Introduces explicit escape hatches and new API surface. Adds add_new_implementation_unsafe (skips validation; required for finalizing upgrades) plus new error codes/sentinel panic handling for validation failures; updates and expands tests with a new ReplaceabilityMockV2, additional helpers, and coverage for non-upgradeable targets, broken EIC, and finalized-contract behavior.

^{Reviewed by Cursor Bugbot for commit 36bffe4. Bugbot is set up for automated code reviews on this repo. Configure here.}

[remollemo]

@remollemo made 12 comments and resolved 2 discussions.
Reviewable status: 1 of 7 files reviewed, 10 unresolved discussions (waiting on liorgold2 and remollemo).

---

packages/utils/src/components/replaceability/errors.cairo line 13 at r2 (raw file):

Previously, liorgold2 wrote…

What's B2A?

B to A (replacing from the target impl back to the source impl)

---

packages/utils/src/components/replaceability/interface.cairo line 5 at r2 (raw file):

Previously, liorgold2 wrote…

Should it be here or in the impl file?

neither, IMO. it's really self explanatory. at the calling site in the impl, some explanation needed, but it's already there.

---

packages/utils/src/components/replaceability/interface.cairo line 46 at r2 (raw file):

Previously, liorgold2 wrote…

Document. E.g., A version of add_new_implementation that skips the future upgradability test.

no function is documented here.

---

packages/utils/src/components/replaceability/interface.cairo line 54 at r2 (raw file):

Previously, liorgold2 wrote…

I didn't understand these lines. Are they important?

yes, it is important, i'll rephrase.
it emphasizes why we need the function to stay in the interface (even though it's ugly) so that we don't refactor ourselves out of it.

---

packages/utils/src/components/replaceability/interface.cairo line 54 at r2 (raw file):

Previously, liorgold2 wrote…

Instruct claude not to use —.
Start the doc with what the function does, only later mention it always panics.

:-)

---

packages/utils/src/components/replaceability/replaceability.cairo line 76 at r2 (raw file):

Previously, liorgold2 wrote…

Maybe move doc to the trait?

i am not sure i follow

---

packages/utils/src/components/replaceability/replaceability.cairo line 77 at r2 (raw file):

Previously, liorgold2 wrote…

WDYM?

the validation performs two upgrades, 1. user planned upgrade "A-->B" 2. upgrade from upgraded code back to current one "B-->A". hence, both directions.

---

packages/utils/src/components/replaceability/replaceability.cairo line 80 at r2 (raw file):

Previously, liorgold2 wrote…

Replace — everywhere in the PR.

Done.

---

packages/utils/src/components/replaceability/replaceability.cairo line 98 at r2 (raw file):

Previously, liorgold2 wrote…

Not sure this comment is helpful (or readable).

Done.

---

packages/utils/src/components/replaceability/replaceability.cairo line 188 at r2 (raw file):

Previously, liorgold2 wrote…

This comment is not clear. Let's discuss.

Done.

---

packages/utils/src/components/replaceability/replaceability.cairo line 215 at r2 (raw file):

Previously, liorgold2 wrote…

Fix comments.

Done.

---

packages/utils/src/components/replaceability/replaceability.cairo line 223 at r2 (raw file):

Previously, liorgold2 wrote…

Add sanity check that the current class hash is now impl_b_hash. This makes sure the EIC didn't upgrade us to another class.

nice

[liorgold2]

@liorgold2 reviewed 3 files and all commit messages, made 4 comments, and resolved 10 discussions.
Reviewable status: 4 of 7 files reviewed, 3 unresolved discussions (waiting on remollemo).

---

packages/utils/src/components/replaceability/replaceability.cairo line 76 at r2 (raw file):

Previously, remollemo wrote…

i am not sure i follow

This relates to the "no function is documented here" you wrote above. I suggested moving all the documentation to the trait instead of the impl. This is usually considered better.

---

packages/utils/src/components/replaceability/replaceability.cairo line 76 at r3 (raw file):

        }

        // Schedule a new implementation upgrade and validates the implementation upgradeability.

(Why did you remove the s? It was according to our conventions: verb + "s" in function docs. Without s in impl comments inside the functions. What the function does vs instructions on what exactly to do)

Suggestion:

        // Schedules a new implementation upgrade and validates the implementation upgradeability.

---

packages/utils/src/components/replaceability/replaceability.cairo line 85 at r3 (raw file):

        }

        // Schedule a new implementation upgrade without validation.

Suggestion:

        // Schedules a new implementation upgrade without validation.

---

packages/utils/src/components/replaceability/replaceability.cairo line 181 at r3 (raw file):

            // impl_a_hash: current class-hash. impl_b_hash: target class-hash.
            let impl_a_hash = get_class_hash_at_syscall(get_contract_address()).unwrap_syscall();
            let impl_b_hash = implementation_data.impl_hash;

So maybe rename, and then you can remove the comment.

Suggestion:

            let current_hash = get_class_hash_at_syscall(get_contract_address()).unwrap_syscall();
            let target_hash = implementation_data.impl_hash;

[liorgold2]

@liorgold2 reviewed 2 files and made 3 comments.
Reviewable status: 6 of 7 files reviewed, 6 unresolved discussions (waiting on remollemo).

---

packages/utils/src/components/replaceability/mock_v2.cairo line 1 at r3 (raw file):

// Variant of `ReplaceabilityMock` with a class hash distinctly different than `mock.cairo`.

I don't see What makes the class hashes different? If it's only the _v2_marker then I prefer that you set it to some value in the constructor to make sure the compiler (or future versions of it) won't optimize it out.

---

packages/utils/src/components/replaceability/test.cairo line 82 at r3 (raw file):

        let mut spy = spy_events();
        replaceable_dispatcher.add_new_implementation(:implementation_data);
        // block_timestamp pinned to 1 in deploy.

Putting cheat_block_timestamp(:contract_address, block_timestamp: 1, span: CheatSpan::Indefinite); here will save the need for the comment.

---

packages/utils/src/components/replaceability/test_utils.cairo line 53 at r3 (raw file):

        )
        .unwrap();
    // Pin block_timestamp > 0 so the validation dry-run yields a non-zero activation_time.

I'm not sure what it does, but given CheatSpan::Indefinite I'd suggest putting these two lines in all the tests that actually need them (assuming its <=4 tests).

[liorgold2]

@liorgold2 reviewed 1 file and made 7 comments.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on remollemo).

---

packages/utils/src/components/replaceability/test.cairo line 442 at r3 (raw file):

        );
        let mut spy = spy_events();
        replaceable_dispatcher.add_new_implementation_unsafe(:implementation_data);

Suggestion:

        // Finalization is only supported by the `*_unsafe` version.
        replaceable_dispatcher.add_new_implementation_unsafe(:implementation_data);

---

packages/utils/src/components/replaceability/test.cairo line 542 at r3 (raw file):

            :contract_address, caller_address: get_upgrade_governor_account(:contract_address),
        );
        match safe_dispatcher.add_new_implementation(:implementation_data) {

Use .expect_err("Should have failed: target has no replaceability component").

Same below.

Consider checking the resulting error.

---

packages/utils/src/components/replaceability/test.cairo line 546 at r3 (raw file):

            Result::Err(_) => (),
        }
    }

At the end of the test, call the unsafe version and make sure it succeeds, possibly completing the upgrade.

---

packages/utils/src/components/replaceability/test.cairo line 607 at r3 (raw file):

            :contract_address, caller_address: get_upgrade_governor_account(:contract_address),
        );
        match safe_dispatcher.add_new_implementation(:implementation_data) {

With "unsafe" it'll pass but the upgrade will fail, right?

---

packages/utils/src/components/replaceability/test.cairo line 614 at r3 (raw file):

    #[test]
    fn test_add_new_implementation_unsafe_succeeds_for_invalid_target() {

Will this test be covered by test_add_new_implementation_blocks_non_upgradeable after you fix my comment? If so, remove it.

---

packages/utils/src/components/replaceability/test.cairo line 638 at r3 (raw file):

    #[test]
    #[should_panic(expected: "ONLY_UPGRADE_GOVERNOR")]
    fn test_add_new_implementation_unsafe_not_upgrade_governor() {

Don't you want to test both safe and unsafe in the same test?

---

packages/utils/src/components/replaceability/test.cairo line 709 at r3 (raw file):

        // Contract is now bricked.
        match safe_dispatcher.add_new_implementation(:implementation_data) {

I think you can move this code also to test_add_new_implementation_blocks_non_upgradeable and remove this test (merge into test_add_new_implementation_blocks_non_upgradeable).

[remollemo]

@remollemo made 10 comments and resolved 4 discussions.
Reviewable status: 3 of 7 files reviewed, 9 unresolved discussions (waiting on liorgold2).

---

packages/utils/src/components/replaceability/mock_v2.cairo line 1 at r3 (raw file):

Previously, liorgold2 wrote…

I don't see What makes the class hashes different? If it's only the _v2_marker then I prefer that you set it to some value in the constructor to make sure the compiler (or future versions of it) won't optimize it out.

Done.

---

packages/utils/src/components/replaceability/replaceability.cairo line 76 at r3 (raw file):

Previously, liorgold2 wrote…

(Why did you remove the s? It was according to our conventions: verb + "s" in function docs. Without s in impl comments inside the functions. What the function does vs instructions on what exactly to do)

I presonally prefer the (not gramatically compliant with present simple) s-less format.

---

packages/utils/src/components/replaceability/test.cairo line 82 at r3 (raw file):

Previously, liorgold2 wrote…

Putting cheat_block_timestamp(:contract_address, block_timestamp: 1, span: CheatSpan::Indefinite); here will save the need for the comment.

Done.

---

packages/utils/src/components/replaceability/test.cairo line 542 at r3 (raw file):

Previously, liorgold2 wrote…

Use .expect_err("Should have failed: target has no replaceability component").

Same below.

Consider checking the resulting error.

Done.

---

packages/utils/src/components/replaceability/test.cairo line 546 at r3 (raw file):

Previously, liorgold2 wrote…

At the end of the test, call the unsafe version and make sure it succeeds, possibly completing the upgrade.

covered already in test_intentional_brick_via_unsafe_add

---

packages/utils/src/components/replaceability/test.cairo line 607 at r3 (raw file):

Previously, liorgold2 wrote…

With "unsafe" it'll pass but the upgrade will fail, right?

yes. as the EIC throws when called (at upgrade or at validation)
added that to the flow.

---

packages/utils/src/components/replaceability/test.cairo line 614 at r3 (raw file):

Previously, liorgold2 wrote…

Will this test be covered by test_add_new_implementation_blocks_non_upgradeable after you fix my comment? If so, remove it.

Done.

---

packages/utils/src/components/replaceability/test.cairo line 638 at r3 (raw file):

Previously, liorgold2 wrote…

Don't you want to test both safe and unsafe in the same test?

well,,, there are many ONLY* tests with a single failure (as they written before safe_dipatcher was intrudocued)
I don't a desire to refactor this in this PR.

---

packages/utils/src/components/replaceability/test.cairo line 709 at r3 (raw file):

Previously, liorgold2 wrote…

I think you can move this code also to test_add_new_implementation_blocks_non_upgradeable and remove this test (merge into test_add_new_implementation_blocks_non_upgradeable).

I will merge the two.

---

packages/utils/src/components/replaceability/test_utils.cairo line 53 at r3 (raw file):

Previously, liorgold2 wrote…

I'm not sure what it does, but given CheatSpan::Indefinite I'd suggest putting these two lines in all the tests that actually need them (assuming its <=4 tests).

I changed it already. it was a way to ensure we don't have enable_time == 0
(would be the case if upgrade_delay == 0, current_block_timestamp == 0)
enable_time == 0 would fool the component code, to assume impl is unknown.

[liorgold2]

@liorgold2 reviewed 4 files and all commit messages, made 1 comment, and resolved 9 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on remollemo).

---

packages/utils/src/components/replaceability/replaceability.cairo line 76 at r3 (raw file):

Previously, remollemo wrote…

I presonally prefer the (not gramatically compliant with present simple) s-less format.

Theoretically, both are fine, but we should pick one so that everyone is consistent.

We copied our conversion from Google: https://google.github.io/styleguide/cppguide.html#Function_Comments
Ours: https://github.com/starkware-industries/starkware/wiki/Python-style-guide#documentation

[liorgold2]

@liorgold2 made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on remollemo).