Skip to content

Conversation

bidzyyys
Copy link
Collaborator

@bidzyyys bidzyyys commented Jun 30, 2025

Resolves #14

Implementing StorageSlot requires our library depend directly on stylus-test feature flag to properly instantiate VM (see source code). But due to how this feature is enabled and what cargo-stylus operations depend on it, it causes great difficulty in synchronizing all of the requirements to make everything work as expected (and as before).
This it made it necessary to make some changes to make our CI pass, but it unfortunately still made export-abi not work at all.
We may find a way to address this in a maintainable way that makes export-abi work before SDK v0.10.0, but for now this will do.

PR Checklist

  • Tests
  • Documentation
  • Changelog

@bidzyyys bidzyyys linked an issue Jun 30, 2025 that may be closed by this pull request
Copy link

netlify bot commented Jun 30, 2025

Deploy Preview for contracts-stylus canceled.

Name Link
🔨 Latest commit 6341344
🔍 Latest deploy log https://app.netlify.com/projects/contracts-stylus/deploys/687e390510425f0009720f84

Copy link

codecov bot commented Jun 30, 2025

Codecov Report

Attention: Patch coverage is 94.09031% with 89 lines in your changes missing coverage. Please review.

Project coverage is 86.0%. Comparing base (8c720e5) to head (6341344).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
contracts/src/proxy/erc1967/utils.rs 96.2% 20 Missing ⚠️
contracts/src/proxy/beacon/proxy.rs 90.7% 19 Missing ⚠️
contracts/src/proxy/erc1967/mod.rs 89.6% 19 Missing ⚠️
contracts/src/proxy/mod.rs 89.7% 19 Missing ⚠️
contracts/src/access/ownable.rs 58.8% 7 Missing ⚠️
contracts/src/utils/address.rs 83.3% 5 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
contracts/src/access/ownable_two_step.rs 100.0% <ø> (ø)
contracts/src/finance/vesting_wallet.rs 57.2% <ø> (ø)
contracts/src/proxy/beacon/upgradeable.rs 100.0% <100.0%> (ø)
contracts/src/token/erc20/extensions/burnable.rs 100.0% <ø> (ø)
...ntracts/src/token/erc721/extensions/consecutive.rs 95.6% <ø> (ø)
contracts/src/utils/storage_slot.rs 100.0% <100.0%> (ø)
contracts/src/utils/address.rs 83.3% <83.3%> (ø)
contracts/src/access/ownable.rs 89.1% <58.8%> (-8.5%) ⬇️
contracts/src/proxy/beacon/proxy.rs 90.7% <90.7%> (ø)
contracts/src/proxy/erc1967/mod.rs 89.6% <89.6%> (ø)
... and 2 more

@bidzyyys
Copy link
Collaborator Author

Blocked by OpenZeppelin/stylus-test-helpers#104

@0xNeshi
Copy link
Collaborator

0xNeshi commented Jul 11, 2025

Blocked by OpenZeppelin/stylus-test-helpers#104

This isn't a blocker per se, we can still test proxies with e2e tests; we can even consider e2e tests to be more reliable, given how fragile proxy logic is.

bidzyyys pushed a commit that referenced this pull request Jul 17, 2025
added a couple of missing constructor-related unit tests

necessary for #729
Copy link
Collaborator Author

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @0xNeshi 👏

Copy link
Member

@qalisander qalisander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job @0xNeshi !
I feel we're good to merge as it is while addressing todo's later.
Just a few notes I have

Comment on lines 652 to 656
// TODO: enable this test when we have a way to test for unknown selector
// errors
//
// #[motsu::test]
// fn upgrade_to_and_call_with_delegate_call_failure(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one should work:

    #[motsu::test]
    fn upgrade_to_and_call_with_delegate_call_failure(
        contract: Contract<TestContract>,
        implementation: Contract<Implementation>,
        alice: Address,
    ) {
        let data = function_selector!("nonExistentFunction").to_vec();

        let err = contract
            .sender(alice)
            .test_upgrade_to_and_call(
                implementation.address(),
                data.clone().into(),
            )
            .expect_err("should fail when delegate call fails");

        let vec = format!(
            "function not found for selector '{0}' and no fallback defined",
            u32::from_be_bytes(TryInto::try_into(data).unwrap())
        )
        .as_bytes()
        .to_vec();

        assert_eq!(
            err,
            Error::FailedCallWithReason(address::FailedCallWithReason {
                reason: stylus_sdk::call::Error::Revert(vec).encode().into()
            })
            .encode(),
        );
    }

And we should do it cause of this change in motsu. We replace default stylus-sdk vec![] error (when selector not found) with ours. We need to think how to change motsu to make this test look better

@0xNeshi 0xNeshi merged commit c08cfa9 into main Jul 21, 2025
25 checks passed
@0xNeshi 0xNeshi deleted the feature/proxy-uups branch July 21, 2025 13:15
bidzyyys added a commit that referenced this pull request Jul 21, 2025
Resolves #14

Implementing `StorageSlot` requires our library depend directly on
`stylus-test` feature flag to properly instantiate
[VM](https://docs.rs/stylus-sdk/0.9.0/stylus_sdk/host/struct.VM.html)
(see [source
code](https://github.com/OffchainLabs/stylus-sdk-rs/blob/b7310ef87d05182a946555ae8754f13c97c21c8b/stylus-sdk/src/host/mod.rs)).
But due to how this feature is enabled and what `cargo-stylus`
operations depend on it, it causes great difficulty in synchronizing all
of the requirements to make everything work as expected (and as before).
This it made it necessary to make some changes to make our CI pass, but
it unfortunately still made `export-abi` not work at all.
We may find a way to address this in a maintainable way that makes
export-abi work before SDK v0.10.0, but for now this will do.

<!--
Before merging the pull request all of the following must be completed.
Feel free to submit a PR or Draft PR even if some items are pending.
Some of the items may not apply.
-->

- [x] Tests
- [x] Documentation
- [x] Changelog

---------

Co-authored-by: Nenad <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proxy - UUPS
3 participants