Skip to content

Conversation

@ericnordelo
Copy link
Member

@ericnordelo ericnordelo commented Jul 16, 2025

Fixes #1054

PR Checklist

  • Tests
  • Documentation
  • Added entry to CHANGELOG.md
  • Tried the feature on a public network

turkaturki and others added 18 commits April 3, 2025 11:15
Add verification of indexed keys in event assertions
- Tests will fail if indexed fields are changed in event definitions
-  Must explicitly specify expected indexed fields
…luding AccessControl, Ownable, Account, EthAccount, Votes, Pausable, ERC1155, ERC20, ERC4626, and ERC721. Each test verifies the correct indexed keys for events such as role grants, ownership transfers, and token transfers.
…rki/cairo-contracts into feat/check-indexed-keys-#1054
@ericnordelo ericnordelo changed the title Feat/check indexed keys #1054 Check indexed keys Jul 17, 2025
@ericnordelo ericnordelo requested review from Copilot and immrsd July 28, 2025 11:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements the ability to check indexed keys in event assertions by migrating from component-specific event structures to a generic Event structure containing keys and data arrays. The migration enables more direct assertion of event keys, allowing tests to verify that events contain expected indexed values.

Key changes include:

  • Migration from component-specific event structures to generic Event structures with keys and data arrays
  • Updated Cairo compiler version from 2.11.4 to 2.12.0-rc.2
  • Code formatting consolidation (combining array elements on single lines)

Reviewed Changes

Copilot reviewed 41 out of 44 changed files in this pull request and generated no comments.

File Description
Various test files Migrated event assertions to use generic Event structure with keys/data arrays
Scarb.toml Updated Cairo version to 2.12.0-rc.2
packages/macros/ Updated to handle Cairo 2.12 API changes and removed with_components module
sncast_scripts/ Consolidated array formatting
Comments suppressed due to low confidence (6)

packages/macros/Cargo.toml:11

  • The version "2.12.0-dev.1" is a development version that may not be stable or available in public registries. Consider using a stable release version instead.
cairo-lang-parser = "2.12.0-dev.1"

packages/macros/Cargo.toml:12

  • The version "2.12.0-dev.1" is a development version that may not be stable or available in public registries. Consider using a stable release version instead.
cairo-lang-plugins = "2.12.0-dev.1"

packages/macros/Cargo.toml:13

  • The version "2.12.0-dev.1" is a development version that may not be stable or available in public registries. Consider using a stable release version instead.
cairo-lang-syntax = "2.12.0-dev.1"

packages/macros/Cargo.toml:14

  • The version "2.12.0-dev.1" is a development version that may not be stable or available in public registries. Consider using a stable release version instead.
cairo-lang-defs = "2.12.0-dev.1"

packages/macros/Cargo.toml:15

  • The version "2.12.0-dev.1" is a development version that may not be stable or available in public registries. Consider using a stable release version instead.
cairo-lang-formatter = "2.12.0-dev.1"

packages/macros/Cargo.toml:16

  • The version "2.12.0-dev.1" is a development version that may not be stable or available in public registries. Consider using a stable release version instead.
cairo-lang-starknet-classes = "2.12.0-dev.1"

@ericnordelo
Copy link
Member Author

This PR proposes a different pattern for testing events by constructing the expected one using the generic Event struct instead of the specific one from the components.

We could create a macro to simplify the creation of this (even the creation of the spy event helpers in general).

@codecov
Copy link

codecov bot commented Jul 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.35%. Comparing base (d99887d) to head (6424558).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1472      +/-   ##
==========================================
- Coverage   92.40%   92.35%   -0.05%     
==========================================
  Files          82       82              
  Lines        2265     2264       -1     
==========================================
- Hits         2093     2091       -2     
- Misses        172      173       +1     
Files with missing lines Coverage Δ
...ckages/governance/src/governor/proposal_core.cairo 100.00% <ø> (ø)
packages/merkle_tree/src/merkle_proof.cairo 90.00% <100.00%> (-0.33%) ⬇️
...s/token/src/erc20/extensions/erc4626/erc4626.cairo 83.21% <ø> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d99887d...6424558. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ericnordelo
Copy link
Member Author

Introduced the ExpectedEventTrait

#[generate_trait]
pub impl ExpectedEvent of ExpectedEventTrait {
    /// Creates a new `Event` with empty `keys` and `data` fields.
    fn new() -> Event {
        Event { keys: array![], data: array![] }
    }

    /// Serializes the given value and appends it to the `keys` field.
    fn key<T, +Serde<T>, +Drop<T>>(self: Event, value: T) -> Event {
        let Event { mut keys, data } = self;
        value.serialize(ref keys);
        Event { keys, data }
    }

    /// Serializes the given value and appends it to the `data` field.
    fn data<T, +Serde<T>, +Drop<T>>(self: Event, value: T) -> Event {
        let Event { keys, mut data } = self;
        value.serialize(ref data);
        Event { keys, data }
    }
}

Copy link
Collaborator

@immrsd immrsd left a comment

Choose a reason for hiding this comment

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

Well done, Eric! The syntax really got better and less verbose after introducing the ExpectedEvent trait 👍
I haven't noticed any issues, just left a couple of suggestions for the doc

Also, there are still some places in tests where tests are checked with the old approach, I think we should update them for consistency

@ericnordelo ericnordelo requested a review from immrsd August 15, 2025 10:23
Copy link
Collaborator

@immrsd immrsd left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions
Copy link
Contributor

🧪 Cairo Contract Size Benchmark Diff

🧪 BYTECODE SIZE (felts)

Contract Old New Δ Note
Attacker 138 +138 ✅ NEW
DualCaseAccessControlDefaultAdminRulesMock 8341 +8341 ✅ NEW
DualCaseAccessControlMock 4532 +4532 ✅ NEW
DualCaseAccountMock 2867 +2867 ✅ NEW
DualCaseERC1155Mock 9455 +9455 ✅ NEW
DualCaseERC1155ReceiverMock 2119 +2119 ✅ NEW
DualCaseERC20Mock 6203 +6203 ✅ NEW
DualCaseERC20PermitMock 7834 +7834 ✅ NEW
DualCaseERC721Mock 12768 +12768 ✅ NEW
DualCaseERC721ReceiverMock 1209 +1209 ✅ NEW
DualCaseEthAccountMock 4867 +4867 ✅ NEW
DualCaseOwnableMock 1358 +1358 ✅ NEW
DualCaseTwoStepOwnableMock 2361 +2361 ✅ NEW
ERC20BlockNumberVotesMock 12958 +12958 ✅ NEW
ERC20OptionalTransferPanicMock 5919 +5919 ✅ NEW
ERC20ReentrantMock 8450 +8450 ✅ NEW
ERC20TimestampVotesMock 12958 +12958 ✅ NEW
ERC2981AccessControlMock 6002 +6002 ✅ NEW
ERC2981Mock 2080 +2080 ✅ NEW
ERC2981OwnableMock 4604 +4604 ✅ NEW
ERC4626AssetsFeesMock 17492 +17492 ✅ NEW
ERC4626ExternalVaultMock 13546 +13546 ✅ NEW
ERC4626LimitsMock 13552 +13552 ✅ NEW
ERC4626Mock 12960 +12960 ✅ NEW
ERC4626MockWithHooks 12934 +12934 ✅ NEW
ERC4626OffsetMock 13464 +13464 ✅ NEW
ERC4626SharesFeesMock 17131 +17131 ✅ NEW
ERC721BlockNumberVotesMock 18786 +18786 ✅ NEW
ERC721EnumerableMock 10510 +10510 ✅ NEW
ERC721TimestampVotesMock 18786 +18786 ✅ NEW
GovernorMock 18513 +18513 ✅ NEW
GovernorQuorumFractionMock 23009 +23009 ✅ NEW
GovernorTimelockedMock 23507 +23507 ✅ NEW
InitializableMock 291 +291 ✅ NEW
LegacyERC20VotesMock 9272 +9272 ✅ NEW
LinearVestingMock 5198 +5198 ✅ NEW
MockContract 323 +323 ✅ NEW
MockTrace 4444 +4444 ✅ NEW
MultisigTargetMock 349 +349 ✅ NEW
MultisigWalletMock 10458 +10458 ✅ NEW
NonImplementingMock 96 +96 ✅ NEW
NoncesMock 215 +215 ✅ NEW
PausableMock 643 +643 ✅ NEW
ReentrancyMock 1473 +1473 ✅ NEW
SRC5Mock 235 +235 ✅ NEW
SRC9AccountMock 5577 +5577 ✅ NEW
SimpleMock 541 +541 ✅ NEW
SnakeAccountMock 3198 +3198 ✅ NEW
SnakeERC1155Mock 7667 +7667 ✅ NEW
SnakeERC1155MockWithHooks 7911 +7911 ✅ NEW
SnakeERC20Mock 5550 +5550 ✅ NEW
SnakeERC20MockWithHooks 5842 +5842 ✅ NEW
SnakeERC721Mock 10685 +10685 ✅ NEW
SnakeERC721MockWithHooks 10850 +10850 ✅ NEW
SnakeEthAccountMock 5221 +5221 ✅ NEW
StepsVestingMock 5408 +5408 ✅ NEW
TimelockAttackerMock 777 +777 ✅ NEW
TimelockControllerMock 10502 +10502 ✅ NEW
UpgradesV1 1074 +1074 ✅ NEW
UpgradesV2 1221 +1221 ✅ NEW

🧪 SIERRA CONTRACT CLASS SIZE (bytes)

Contract Old New Δ Note
Attacker 6085 +6085 ✅ NEW
DualCaseAccessControlDefaultAdminRulesMock 171622 +171622 ✅ NEW
DualCaseAccessControlMock 96706 +96706 ✅ NEW
DualCaseAccountMock 66378 +66378 ✅ NEW
DualCaseERC1155Mock 217375 +217375 ✅ NEW
DualCaseERC1155ReceiverMock 38147 +38147 ✅ NEW
DualCaseERC20Mock 128806 +128806 ✅ NEW
DualCaseERC20PermitMock 177200 +177200 ✅ NEW
DualCaseERC721Mock 274742 +274742 ✅ NEW
DualCaseERC721ReceiverMock 24342 +24342 ✅ NEW
DualCaseEthAccountMock 91233 +91233 ✅ NEW
DualCaseOwnableMock 30723 +30723 ✅ NEW
DualCaseTwoStepOwnableMock 47987 +47987 ✅ NEW
ERC20BlockNumberVotesMock 291969 +291969 ✅ NEW
ERC20OptionalTransferPanicMock 124793 +124793 ✅ NEW
ERC20ReentrantMock 189745 +189745 ✅ NEW
ERC20TimestampVotesMock 291932 +291932 ✅ NEW
ERC2981AccessControlMock 123006 +123006 ✅ NEW
ERC2981Mock 40853 +40853 ✅ NEW
ERC2981OwnableMock 93132 +93132 ✅ NEW
ERC4626AssetsFeesMock 379741 +379741 ✅ NEW
ERC4626ExternalVaultMock 293564 +293564 ✅ NEW
ERC4626LimitsMock 295692 +295692 ✅ NEW
ERC4626Mock 281128 +281128 ✅ NEW
ERC4626MockWithHooks 277901 +277901 ✅ NEW
ERC4626OffsetMock 293332 +293332 ✅ NEW
ERC4626SharesFeesMock 369762 +369762 ✅ NEW
ERC721BlockNumberVotesMock 432502 +432502 ✅ NEW
ERC721EnumerableMock 226427 +226427 ✅ NEW
ERC721TimestampVotesMock 432493 +432493 ✅ NEW
GovernorMock 457408 +457408 ✅ NEW
GovernorQuorumFractionMock 556705 +556705 ✅ NEW
GovernorTimelockedMock 579123 +579123 ✅ NEW
InitializableMock 9238 +9238 ✅ NEW
LegacyERC20VotesMock 210979 +210979 ✅ NEW
LinearVestingMock 103157 +103157 ✅ NEW
MockContract 8400 +8400 ✅ NEW
MockTrace 87974 +87974 ✅ NEW
MultisigTargetMock 9120 +9120 ✅ NEW
MultisigWalletMock 228078 +228078 ✅ NEW
NonImplementingMock 4165 +4165 ✅ NEW
NoncesMock 7019 +7019 ✅ NEW
PausableMock 17655 +17655 ✅ NEW
ReentrancyMock 32174 +32174 ✅ NEW
SRC5Mock 8385 +8385 ✅ NEW
SRC9AccountMock 129558 +129558 ✅ NEW
SimpleMock 14879 +14879 ✅ NEW
SnakeAccountMock 74308 +74308 ✅ NEW
SnakeERC1155Mock 181167 +181167 ✅ NEW
SnakeERC1155MockWithHooks 190917 +190917 ✅ NEW
SnakeERC20Mock 116477 +116477 ✅ NEW
SnakeERC20MockWithHooks 142850 +142850 ✅ NEW
SnakeERC721Mock 232720 +232720 ✅ NEW
SnakeERC721MockWithHooks 241047 +241047 ✅ NEW
SnakeEthAccountMock 100099 +100099 ✅ NEW
StepsVestingMock 107175 +107175 ✅ NEW
TimelockAttackerMock 22711 +22711 ✅ NEW
TimelockControllerMock 250722 +250722 ✅ NEW
UpgradesV1 25813 +25813 ✅ NEW
UpgradesV2 28303 +28303 ✅ NEW

This comment was generated automatically from benchmark diffs.

@ericnordelo ericnordelo merged commit 26fc40c into main Aug 18, 2025
10 checks passed
@ericnordelo ericnordelo deleted the feat/check-indexed-keys-#1054 branch August 18, 2025 14:42
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.

Check indexed keys in event assertions (snforge migration)

4 participants