Skip to content

Conversation

@drewstone
Copy link
Contributor

  • Fix TxBaseImplication constructor usage in transaction extension tests
  • Add #[allow(dead_code)] for unused test mock utilities
  • Replace manual absolute difference with .abs_diff() method
  • Convert test constants to uppercase (Alice -> ALICE, etc)
  • Add missing 11th parameter (authorization_list) to Evm::call for EIP-7702Summary of changes

- Fix TxBaseImplication constructor usage in transaction extension tests
- Add #[allow(dead_code)] for unused test mock utilities
- Replace manual absolute difference with .abs_diff() method
- Convert test constants to uppercase (Alice -> ALICE, etc)
- Add missing 11th parameter (authorization_list) to Evm::call for EIP-7702
Replace () with FreeEVMExecution for OnChargeTransaction type to properly
utilize the mock implementation and eliminate dead code warning.
Remove ExtBuilder and MockedEvmRunner that were never used.
These were copy-pasted boilerplate from the original PR but
tests use new_test_ext() directly without needing ExtBuilder,
and nothing references MockedEvmRunner.
- Add DoneSlashHandler to pallet_balances::Config
- Add EVM config types (AccountProvider, CreateOriginFilter, CreateInnerOriginFilter, GasLimitStorageGrowthRatio)
- Add Holder type to pallet_assets::Config
- Add dev_accounts field to GenesisConfig
- Update migrations to use new storage API
- Implement DecodeWithMemTracking trait where needed
@claude
Copy link

claude bot commented Oct 12, 2025

Claude finished @drewstone's task —— View job


PR Review: Update all tests/mocks

I've completed a comprehensive review of this PR that updates tests and mocks across 40 files. Here's my detailed analysis:

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Review EVM-related changes (11th parameter addition)
  • Analyze mock configuration updates
  • Check test constant naming changes (Alice -> ALICE)
  • Review dead code annotations and cleanup
  • Examine abs_diff() replacements
  • Check for security implications
  • Provide comprehensive feedback

Code Quality & Best Practices

Excellent improvements observed:

  1. EIP-7702 Integration: Proper addition of the 11th authorization_list parameter to Evm::call functions enables EIP-7702 account delegation functionality. This is correctly implemented as vec![] (empty authorization list) for existing test cases.

  2. Code Modernization: Replacing manual absolute difference calculations with the built-in .abs_diff() method improves readability and leverages Rust's standard library (seen in pallets/rewards/src/tests/claim.rs:159).

  3. Test Constants: Converting test constants from lowercase to uppercase (AliceALICE) follows Rust naming conventions for constants and improves code consistency.

  4. Dead Code Management: Adding #[allow(dead_code)] attributes and cleaning up unused test utilities is a good maintenance practice that reduces compiler warnings.

Security Analysis

No security concerns identified:

  1. EVM Parameter Addition: The authorization list parameter addition is secure - empty vectors are safe defaults for existing functionality
  2. Mock Updates: Configuration changes only affect test environments, not production runtime
  3. Test Isolation: All changes maintain proper test isolation and don't expose sensitive functionality

Performance Considerations

Minor performance improvements:

  1. .abs_diff() Usage: More efficient than manual absolute difference calculations
  2. Mock Cleanups: Removing unused code (SuicideQuickClearLimit) reduces test overhead
  3. EVM Configuration: Updates align with Polkadot SDK optimizations

🔍 Areas for Attention

  1. TxBaseImplication Usage: The sp_runtime::traits::TxBaseImplication(()) pattern in native restaking tests (lines 178, 702, 766, 844 in pallets/multi-asset-delegation/src/tests/native_restaking.rs) appears correct for transaction extension testing.

  2. Mock State Root: The switch from IntermediateStateRoot<Self> to MockStateRoot in EVM mocks simplifies testing but ensure this covers all required test scenarios.

  3. Missing Config Fields: The addition of new pallet configuration fields like DoneSlashHandler = () suggests dependency updates - verify all required fields are properly configured.

📋 Specific File Feedback

  • pallets/services/src/mock_evm.rs: Well-structured EVM mock with proper function selector handling
  • precompiles/*/src/mock.rs: Consistent configuration updates across precompiles
  • Test files: Good use of constants and improved assertion patterns

Documentation

No documentation updates appear needed as these are internal test/mock changes that don't affect public APIs.

🎯 Recommendation: APPROVE

This PR demonstrates excellent maintenance practices with:

  • ✅ Proper EIP-7702 integration
  • ✅ Code quality improvements
  • ✅ Security-conscious changes
  • ✅ Performance optimizations
  • ✅ Consistent naming conventions

The changes are well-contained to test environments and represent a clean update to align with Polkadot SDK changes.


@1xstj 1xstj merged commit cbba48f into 1xstj/bump-polkadot-sdk Oct 13, 2025
1 check passed
@1xstj 1xstj deleted the drew/bump-polkadot-sdk branch October 13, 2025 10:55
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.

3 participants