Skip to content

Use OZ 0.8.1 implementation of STRK and ETH ERC20 token contracts#750

Merged
FabijanC merged 3 commits intomainfrom
revert-erc20
Apr 9, 2025
Merged

Use OZ 0.8.1 implementation of STRK and ETH ERC20 token contracts#750
FabijanC merged 3 commits intomainfrom
revert-erc20

Conversation

@FabijanC
Copy link
Copy Markdown
Contributor

@FabijanC FabijanC commented Apr 8, 2025 •
edited
Loading

Usage related changes

Development related changes

Checklist:

  • Checked out the contribution guidelines
  • Applied formatting - ./scripts/format.sh
  • No linter errors - ./scripts/clippy_check.sh
  • No unused dependencies - ./scripts/check_unused_deps.sh
  • No spelling errors - ./scripts/check_spelling.sh
  • Performed code self-review
  • Rebased to the latest commit of the target branch (or merged it into my branch)
    • Once you make the PR reviewable, please avoid force-pushing
  • Updated the docs if needed - ./website/README.md
  • Linked the issues resolvable by this PR - linking info
  • Updated the tests if needed; all passing - execution info

Summary by CodeRabbit

  • Chores
    • Removed outdated compilation records and updated contract references to a newer ERC20 version.
  • Refactor
    • Streamlined the token initialization process by integrating string conversion directly into setup.
  • Tests
    • Enhanced integration tests to validate ERC20 token properties, ensuring token names and symbols match expected values.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 8, 2025 •
edited
Loading

Walkthrough

This pull request removes an outdated compilation record file and updates constants to reference a new ERC20 contract version (from 0.20.0 to 0.8.1) along with its corresponding class hash. It refactors the ERC20 initialization process in the predeployed contracts by removing an intermediary string conversion function and integrating its logic directly. Additionally, it updates integration tests to validate the ERC20 token name and symbol instead of decimals, and removes unused imports.

Changes

File(s) Change Summary
crates/starknet-devnet-core/contracts/.../compilation_info.txt Removed file that recorded ERC20 contract (v0.20.0) compilation details.
crates/starknet-devnet-core/src/constants.rs, tests/integration/common/constants.rs Updated ERC20 contract reference from version 0.20.0 to 0.8.1 and replaced the associated class hash with a new value.
crates/starknet-devnet-core/src/starknet/predeployed.rs Removed the store_short_string_as_byte_array function and integrated its string-to-felt conversion logic into initialize_erc20_at_address.
tests/integration/general_integration_tests.rs Refactored tests to assert on ERC20 name and symbol instead of decimals, and removed obsolete imports.

Sequence Diagram(s)

sequenceDiagram
    participant I as initialize_erc20_at_address
    participant L as Loop (name & symbol)
    participant C as parse_cairo_short_string
    participant S as Contract Storage

    I->>L: Iterate over token properties (name, symbol)
    L->>C: Convert each property to felt value
    C-->>L: Return felt conversion or error
    L->>S: Store converted felt value in storage
Loading

Assessment against linked issues

Objective Addressed Explanation
ERC20 contracts predeployed with valid name and symbol (#739) ✅

Possibly related PRs

Suggested reviewers

  • marioiordanov

Poem

I hopped through lines of code tonight,
Removing old files with carrot-like might.
Constants gleam in a fresh new light,
Tests now sing with names so bright.
As a rabbit, I cheer with ASCII delight! 🥕

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b43f367 and c8e7dba.

📒 Files selected for processing (1)
  • tests/integration/general_integration_tests.rs (4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/integration/general_integration_tests.rs (1)
crates/starknet-devnet-core/src/utils.rs (1)
  • get_storage_var_address (31-40)
🔇 Additional comments (4)
tests/integration/general_integration_tests.rs (4)

2-5: Imports updated to align with OZ 0.8.1 implementation

The addition of parse_cairo_short_string from starknet_rs_core::utils aligns with the PR objective of using the OZ 0.8.1 implementation of ERC20 token contracts with Felt properties rather than ByteArray.

113-116: Test cases updated to validate name and symbol

This change aligns with the PR goals by:

  1. Focusing on validating the name and symbol properties of ERC20 tokens
  2. Removing the previous validation of the "decimals" property
  3. Ensuring the predeploy configuration matches expectations for both ETH and STRK tokens

The updated test data is consistent with standard values used on Testnet and Mainnet.

128-128: String parsing approach used for storage values

The use of parse_cairo_short_string properly handles the conversion from Felt to string representation, allowing for direct comparison with expected string values. This aligns with the PR objective of using Felt properties rather than ByteArray.

Consider handling the potential unwrap failure more gracefully for robustness:

- assert_eq!(parse_cairo_short_string(&actual_value).unwrap().as_str(), expected_value);
+ let actual_string = parse_cairo_short_string(&actual_value)
+     .unwrap_or_else(|e| panic!("Failed to parse storage value: {}", e));
+ assert_eq!(actual_string.as_str(), expected_value);

154-155: ERC20 property getter validation updated

The changes correctly verify that:

  1. The getters return exactly one value
  2. The returned value, when parsed as a Cairo short string, matches the expected token name/symbol

This properly reflects the OZ 0.8.1 implementation which returns Felt values instead of ByteArray.

Consider handling the unwrap operation more defensively:

  assert_eq!(actual_felts.len(), 1);
- assert_eq!(parse_cairo_short_string(&actual_felts[0]).unwrap(), expected_value);
+ match parse_cairo_short_string(&actual_felts[0]) {
+     Ok(actual_string) => assert_eq!(actual_string, expected_value),
+     Err(e) => panic!("Failed to parse '{}' getter result: {}", getter_name, e),
+ }
✨ Finishing Touches
  • 📝 Generate Docstrings
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@FabijanC FabijanC requested a review from marioiordanov April 8, 2025 14:26
@FabijanC FabijanC merged commit dd80b3f into main Apr 9, 2025
2 checks passed
@FabijanC FabijanC deleted the revert-erc20 branch April 9, 2025 07:12
@coderabbitai coderabbitai bot mentioned this pull request Jun 13, 2025
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@marioiordanov marioiordanov marioiordanov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ERC20 contracts predeployed with invalid name and symbol

2 participants

@FabijanC @marioiordanov