Skip to content

Fix ByteArray handling in ERC20 predeployment#744

Merged
FabijanC merged 2 commits intomainfrom
fix-erc20-predeployment
Apr 4, 2025
Merged

Fix ByteArray handling in ERC20 predeployment#744
FabijanC merged 2 commits intomainfrom
fix-erc20-predeployment

Conversation

@FabijanC
Copy link
Copy Markdown
Contributor

@FabijanC FabijanC commented Apr 4, 2025 •
edited by coderabbitai bot
Loading

Usage related changes

Development related changes

  • Predeploy properties of type ByteArray

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

  • New Features
    • Introduced an improved mechanism for converting and storing short text values, enhancing the initialization process for token contracts.
  • Tests
    • Updated integration tests to verify proper storage and retrieval of token properties, ensuring greater consistency and reliability.

@FabijanC FabijanC requested a review from marioiordanov April 4, 2025 12:41
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 4, 2025 •
edited
Loading

Walkthrough

The pull request introduces a new utility function to handle the conversion and storage of short strings as byte arrays within a Starknet contract’s state. The function encapsulates the conversion of strings to felt values using a utility function and a Poseidon hash for determining storage addresses. The ERC20 contract initialization code has been modified to leverage this function for setting name and symbol values. Additionally, the integration tests have been updated to reflect new storage expectations and include a new test for validating property retrieval.

Changes

File(s) Change Summary
.../starknet/predeployed.rs Added store_short_string_as_byte_array that converts a short string to a felt value, computes a Poseidon hash for a storage chunk base, and stores the value. Updated initialize_erc20_at_address to use this helper, replacing earlier manual storage operations for "ERC20_name" and "ERC20_symbol".
.../integration_tests.rs Modified imports and updated predeployed_erc20_tokens_have_expected_storage to reflect new storage variable expectations. Added a new test predeployed_erc20_tokens_return_expected_values_from_property_getters to verify ERC20 token property getters.

Sequence Diagram(s)

sequenceDiagram
    participant ERC20Init as initialize_erc20_at_address
    participant StorageFunc as store_short_string_as_byte_array
    participant CairoUtil as cairo_short_string_to_felt
    participant Poseidon as PoseidonHash
    participant State as StarknetState

    ERC20Init->>StorageFunc: Call(state, contract_addr, storage_var, short_str)
    StorageFunc->>CairoUtil: Convert short string to felt
    CairoUtil-->>StorageFunc: Return felt value
    StorageFunc->>Poseidon: Compute capacity & chunk base hash
    Poseidon-->>StorageFunc: Return chunk base
    StorageFunc->>State: Write felt value to storage at chunk base
    StorageFunc-->>ERC20Init: Return success
Loading

Assessment against linked issues

Objective Addressed Explanation
Ensure ERC20 contracts predeploy correct ByteArray-based properties for name and symbol (#739) ✅

Poem

I'm a rabbit in a code-lined glade,
Hopping through bytes in a merry parade.
Converting strings with a flick and a dash,
Ensuring ERC20 tokens now smoothly flash.
My whiskers twitch with each improved line—
Hoppy changes make the code just fine!

📜 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 68b7b69 and 96052ed.

📒 Files selected for processing (2)
  • crates/starknet-devnet-core/src/starknet/predeployed.rs (3 hunks)
  • tests/integration/general_integration_tests.rs (3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/integration/general_integration_tests.rs (6)
crates/starknet-devnet-core/src/utils.rs (1)
  • get_storage_var_address (31-40)
tests/integration/test_advancing_time.rs (1)
  • devnet (178-184)
tests/integration/common/background_devnet.rs (1)
  • spawn (91-93)
tests/integration/common/background_anvil.rs (1)
  • spawn (33-35)
tests/integration/common/utils.rs (2)
  • from (596-611)
  • from (615-639)
crates/starknet-devnet-types/src/rpc/contract_class.rs (3)
  • from (62-64)
  • from (68-70)
  • from (74-76)
🔇 Additional comments (6)
crates/starknet-devnet-core/src/starknet/predeployed.rs (3)

6-6: New import added for ByteArray hashing.

The Poseidon hash is now imported from the starknet_types_core crate to support the ByteArray storage implementation.

26-58: Well-implemented utility for ByteArray storage.

This new utility function correctly implements the storage of short strings as byte arrays in Starknet contracts. The implementation follows the Cairo ByteArray structure, which requires:

  1. Storing the length at the storage address
  2. Computing a hashed chunk base for storing the actual string data
  3. Setting the felt value at the chunk base

Good error handling and comprehensive comments explaining the non-trivial aspects of ByteArray storage.

69-71: Improved code organization for ERC20 initialization.

The previous direct storage implementation has been replaced with a more modular approach using the new store_short_string_as_byte_array utility. This refactoring improves code maintainability while handling string values properly as ByteArrays.

tests/integration/general_integration_tests.rs (3)

3-5: Updated imports to support ByteArray handling.

Added necessary imports for Decode, ByteArray and FunctionCall to support the new test for property getters, while removing the now-unused parse_cairo_short_string import.

190-211: Refactored test to focus on decimal storage verification.

This test has been appropriately refactored to focus solely on decimals storage verification, as noted in the comment. The name and symbol validation has been moved to the new dedicated test for property getters. The assertion has been simplified by directly comparing the values.

213-239: Well-designed test for ERC20 property validation.

This new test complements the refactoring done in the codebase by validating that the name and symbol properties are correctly stored and can be retrieved via contract function calls. The implementation:

  1. Calls the appropriate getter functions on the contracts
  2. Properly decodes the returned ByteArray into a string
  3. Verifies against expected values for both ETH and STRK tokens

This is a more robust validation than just checking the raw storage values.

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 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 changed the title Fix ERC20 token contract predeployment Fix ByteArray handling in ERC20 token contract predeployment Apr 4, 2025
@FabijanC FabijanC changed the title Fix ByteArray handling in ERC20 token contract predeployment Fix ByteArray handling in ERC20 predeployment Apr 4, 2025
@FabijanC FabijanC merged commit dd39de8 into main Apr 4, 2025
2 checks passed
@FabijanC FabijanC deleted the fix-erc20-predeployment branch April 4, 2025 13:39
FabijanC added a commit that referenced this pull request Apr 8, 2025
@FabijanC
Revert "Fix ByteArray handling in ERC20 predeployment (#744)"
b43f367 
This reverts commit dd39de8.
@FabijanC FabijanC mentioned this pull request Apr 8, 2025
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marioiordanov marioiordanov Awaiting requested review from marioiordanov

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

1 participant

@FabijanC