tangle icon indicating copy to clipboard operation
tangle copied to clipboard

chore : bump polkadot-sdk

Open drewstone opened this issue 2 months ago • 2 comments

  • chore: fix clippy warnings for stable2503
  • 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
  • chore: use FreeEVMExecution in EVM config

Replace () with FreeEVMExecution for OnChargeTransaction type to properly utilize the mock implementation and eliminate dead code warning.

  • chore: remove unused dead code from rewards mocks

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.

  • chore: update remaining mocks for polkadot-sdk stable2503
  • 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 neededSummary of changes Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes

drewstone avatar Oct 29 '25 12:10 drewstone

@danielbui12 we'll want to finish this too, lets chat about what you're stuck on.

drewstone avatar Oct 29 '25 12:10 drewstone

Claude finished @drewstone's task —— View job


PR Review: Polkadot SDK Upgrade to stable2503

Review Tasks:

  • [x] Read repository guidelines (CLAUDE.md)
  • [x] Analyze core runtime and pallet changes
  • [x] Review EVM integration updates
  • [x] Check migration safety and compatibility
  • [x] Examine security implications
  • [x] Review testing and mock updates
  • [x] Provide comprehensive feedback

Overall Assessment

This is a comprehensive upgrade to Polkadot SDK stable2503 with significant breaking changes. The changes appear well-structured and follow Substrate migration best practices. Here's my detailed feedback:

✅ Positive Aspects

1. Comprehensive Migration Strategy

  • Well-structured migration: pallets/multi-asset-delegation/src/migrations.rs:31-283 shows a thorough migration with proper pre/post upgrade checks
  • Backwards compatibility: Migration preserves existing data while adding new fields like is_nomination
  • Error handling: Proper error handling with detailed logging for failed migrations

2. EVM Integration Updates

  • EIP-7702 Support: Added authorization_list parameter throughout EVM calls for EIP-7702 compatibility
  • Consistent implementation: The parameter is properly added across all runtime call sites and mock implementations
  • Future-proofing: Using _authorization_list prefix indicates awareness it's not yet fully utilized

3. Code Quality Improvements

  • Modern Rust patterns: Replaced manual absolute difference calculations with .abs_diff() method
  • Proper constants: Test constants converted to uppercase following Rust conventions
  • Memory tracking: Added DecodeWithMemTracking implementations where required

⚠️ Areas of Concern

1. Migration Safety - Medium Priority

Location: pallets/multi-asset-delegation/src/migrations.rs:130-158

let raw_value = match unhashed::get_raw(&raw_key) {
    Some(bytes) => bytes,
    None => {
        log::warn!("No raw metadata found for account: {:?}", account_id);
        continue;
    },
};

Issue: Migration continues on missing raw metadata, but this could indicate data corruption or unexpected storage state. Recommendation: Consider making this a hard error or implementing more robust validation.

2. Potential Data Loss Risk - High Priority

Location: pallets/multi-asset-delegation/src/migrations.rs:205-213

if new_withdraw_requests.try_push(WithdrawRequest { ... }).is_err() {
    log::warn!("Failed to migrate a withdraw request for account {:?}: exceeded capacity", account_id);
    break;
}

Issue: When capacity limits are exceeded, remaining requests are silently dropped without rollback. Recommendation: Either increase capacity limits or implement a two-phase migration approach.

3. EVM Configuration Concerns - Medium Priority

Location: Various mock_evm.rs files

  • FreeEVMExecution: Using free execution in production could be exploited
  • Mock configurations: Some mock files still contain placeholder/development configurations that should be reviewed for production use

4. Missing Test Updates - Medium Priority

Location: pallets/multi-asset-delegation/src/tests/native_restaking.rs:* The TxBaseImplication constructor calls suggest tests may need updates for new transaction extension patterns.

🔒 Security Implications

1. Runtime Upgrade Safety

  • Migration weight calculation: Proper weight accounting in migrations prevents DoS attacks
  • Storage version checks: Missing explicit storage version checks could allow re-running migrations

2. EVM Security

  • Authorization lists: EIP-7702 authorization lists not fully implemented but parameter exists - ensure this doesn't create attack vectors
  • Gas accounting: EVM configuration changes maintain proper gas accounting

3. Consensus Critical

  • Balance configuration: Added DoneSlashHandler = () maintains slashing behavior
  • Transaction extensions: Proper integration with new transaction extension system

📋 Specific Recommendations

1. Immediate Actions

// In migrations.rs, add explicit storage version management:
const MIGRATION_VERSION: StorageVersion = StorageVersion::new(1);

#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::DispatchError> {
    // Verify current version before migration
    ensure!(
        Pallet::<T>::on_chain_storage_version() < MIGRATION_VERSION,
        "Migration already applied"
    );
    // ... rest of pre_upgrade
}

2. Testing Enhancements

  • Add integration tests for the new is_nomination field behavior
  • Test migration edge cases (capacity limits, corrupted data)
  • Verify EVM authorization_list parameter doesn't break existing functionality

3. Documentation Updates

  • Missing: No documentation updates for the breaking changes
  • Recommended: Add migration guide for downstream projects
  • Runtime spec: Update runtime spec version to reflect changes

🚨 Blockers (Must Fix)

1. Missing Storage Version Updates

The migrations don't explicitly update storage versions, which could allow accidental re-execution.

2. Capacity Limit Handling

Migration can lose data when capacity limits are exceeded - this needs robust handling or increased limits.

✅ Nice to Have

  1. Performance: Consider batched migration for large datasets
  2. Monitoring: Add metrics for migration success/failure rates
  3. Rollback: Consider implementing rollback mechanisms for failed migrations

Summary

This is a well-executed Polkadot SDK upgrade with proper attention to migration safety and backwards compatibility. The main concerns are around potential data loss during migration and missing storage version management. With the recommended fixes, this should be safe for deployment.

Approval Status: Conditional - fix the high-priority data loss risk and add storage version management.


claude[bot] avatar Nov 12 '25 15:11 claude[bot]