Skip to content

fix(chain-spec): add deprecated default erc20 class#156

Merged
kariy merged 1 commit intomainfrom
fix/genesis
Jul 4, 2025
Merged

fix(chain-spec): add deprecated default erc20 class#156
kariy merged 1 commit intomainfrom
fix/genesis

Conversation

@kariy
Copy link
Member

@kariy kariy commented Jul 3, 2025

No description provided.

@claude
Copy link
Contributor

claude bot commented Jul 3, 2025

Claude finished @kariy's task —— View job


PR Review Complete ✅

Todo List:

  • Read the changed file to understand the modifications
  • Analyze code quality and best practices
  • Check for potential bugs or issues
  • Review performance considerations
  • Assess security concerns
  • Evaluate test coverage needs
  • Provide constructive feedback with Marcus Aurelius quote

Code Review

Summary

This is a good bug fix that adds a missing line to ensure the ERC20 class is properly marked as a deprecated declared class in the state updates.

Analysis

✅ Code Quality & Best Practices:

  • The change follows the existing pattern in the codebase
  • Consistent with how add_default_udc function handles deprecated classes on line 252
  • Proper placement within the add_default_fee_tokens function

✅ Bug Fix Validation:

  • Issue: The ERC20 class was being added to the classes collection but not to the deprecated_declared_classes set
  • Impact: This would cause the test on line 386-392 to fail, as it expects DEFAULT_LEGACY_ERC20_CLASS_HASH to be present in deprecated_declared_classes
  • Fix: Adding states.state_updates.deprecated_declared_classes.insert(DEFAULT_LEGACY_ERC20_CLASS_HASH); resolves the inconsistency

✅ Test Coverage:

  • Existing comprehensive test at /crates/chain-spec/src/dev.rs:386-392 validates this behavior
  • Test explicitly checks for the ERC20 class hash in the deprecated_declared_classes collection
  • This PR likely fixes a failing test

✅ Performance:

  • Minimal performance impact - single HashMap insertion operation
  • No additional memory allocation concerns

✅ Security:

  • No security implications
  • Uses existing constants and follows established patterns

Recommendations

  1. Consider adding a comment explaining why both legacy classes (ERC20 and UDC) need to be marked as deprecated
  2. Commit message is clear and accurately describes the fix

Verdict

LGTM - This is a clean, necessary bug fix that maintains consistency with the existing codebase patterns and satisfies the test requirements.

"Look well into thyself; there is a source of strength which will always spring up if thou wilt always look."
- Marcus Aurelius, Meditations

@github-actions
Copy link

github-actions bot commented Jul 3, 2025

Binary size report 📊

Branch Size
main 64.08MiB
fix/genesis 64.08MiB ( +0.00% )

@codecov
Copy link

codecov bot commented Jul 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.22%. Comparing base (9bde0ae) to head (79e61b0).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #156      +/-   ##
==========================================
- Coverage   73.32%   72.22%   -1.10%     
==========================================
  Files         209      217       +8     
  Lines       23132    23621     +489     
==========================================
+ Hits        16961    17060      +99     
- Misses       6171     6561     +390     

☔ 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.

@kariy kariy merged commit e6aaa10 into main Jul 4, 2025
12 of 13 checks passed
@kariy kariy deleted the fix/genesis branch July 4, 2025 09:50
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.

1 participant