-
Notifications
You must be signed in to change notification settings - Fork 184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: update-erc20-class #2521
feat: update-erc20-class #2521
Conversation
taking a look into the error |
@enitrat do you mind recompiling this but without the owner arg ?
considering its not being used at all. just to avoid confusion. |
ah yeah sorry i forgot to delete that one |
ok thanks. the test should pass now |
WalkthroughOhayo, sensei! This pull request introduces a new ERC20 token contract implemented in Cairo for the StarkNet platform. It includes essential functionalities such as token transfers, allowance management, and various view functions to retrieve token properties. Additionally, modifications were made to the executor tests and fixtures to accommodate the new contract, including updates to transaction assertions and deployment structures. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
crates/katana/executor/tests/fixtures/mod.rs (2)
189-210
: Ohayo, sensei! The ERC20 deployment looks solid, but let's make it even better!The updated calldata for deploying an ERC20 contract using UDC is well-structured and follows best practices. It's great to see realistic scenarios in our test fixtures!
However, to make this fixture more flexible, consider parameterizing the ERC20 details (name, symbol, decimals, total supply). This would allow for easier testing of different ERC20 configurations.
Here's a suggestion to make the fixture more reusable:
pub fn create_erc20_deployment_calldata( name: &str, symbol: &str, decimals: u8, total_supply: u128, recipient: ContractAddress, ) -> Vec<Felt> { vec![ felt!("0x1"), felt!("0x41a78e741e5af2fec34b695679bc6891742439f7afb8484ecd7766661ad02bf"), felt!("0x1987cbd17808b9a23693d4de7e246a443cfe37e6e7fbaeabd7d7e6532b07c3d"), felt!("10"), DEFAULT_LEGACY_ERC20_CONTRACT_CLASS_HASH, felt!("0x6ea2ff5aa6f633708e69f5c61d2ac5f860d2435b46ddbd016aa065bce25100a"), felt!("0x1"), felt!("6"), Felt::from_string_str(name).unwrap(), Felt::from_string_str(symbol).unwrap(), Felt::from(decimals), Felt::from(total_supply), Felt::ZERO, recipient.into(), ] }Then, in the
valid_blocks
function:calldata: create_erc20_deployment_calldata( "KARI", "KARI", 18, 6969, sender_address, ),This approach would make our test fixtures more flexible and easier to maintain, sensei!
Line range hint
1-265
: Ohayo, sensei! Overall structure looks good, but let's not forget our TODOs!The changes to incorporate ERC20 contract deployment are well-integrated into the existing structure. Great job on enhancing our test scenarios!
However, I noticed we still have some TODO comments lingering around:
- Line 30: "TODO: remove support for legacy contract declaration"
- Line 93: "TODO: update the txs to include valid signatures"
- Line 233: "TODO: test both with and without the flags turned on"
These TODOs suggest there might be some pending tasks or improvements. Should we create GitHub issues to track these, or are they still relevant? Let me know if you'd like me to help create these issues, sensei!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- crates/katana/contracts/erc20.cairo (1 hunks)
- crates/katana/executor/tests/executor.rs (1 hunks)
- crates/katana/executor/tests/fixtures/mod.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
crates/katana/executor/tests/fixtures/mod.rs (2)
15-16
: Ohayo, sensei! LGTM on the import changes.The addition of
DEFAULT_LEGACY_ERC20_CONTRACT_CLASS_HASH
suggests that we're gearing up for some exciting ERC20 contract deployment scenarios in our test fixtures. Nice touch!
Line range hint
220-231
: Ohayo, sensei! A quick note on thecfg
fixture.While there are no visible changes to the
cfg
fixture, it's worth noting that it usesDEFAULT_FEE_TOKEN_ADDRESS
. This constant is related to the newly importedDEFAULT_LEGACY_ERC20_CONTRACT_CLASS_HASH
.Consider if any updates are needed here to align with the new ERC20 deployment scenario we've introduced. If not, then everything looks good as is!
crates/katana/executor/tests/executor.rs (3)
179-179
: Ohayo, sensei! This change looks sugoi!The updated assertion and comment provide clearer context about the expected number of transactions. It's now explicit that we're accounting for an additional transaction from block 3.
184-184
: Sugoi update, sensei!Replacing the hardcoded felt value with
DEFAULT_LEGACY_ERC20_CONTRACT_CLASS_HASH
is a great improvement. It enhances code maintainability and readability by using a descriptive constant instead of a magic number.
197-197
: Ohayo, sensei! Could you enlighten us about this change?The felt value for the last constructor argument has been updated. While this change might be correct, it would be helpful to understand the reasoning behind it. Could you provide some context on why this specific value was chosen?
✅ Verification successful
Ohayo, sensei! The new felt value is used consistently, and the old value only appeared in a comment. No issues found with this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the new felt value is used consistently across the codebase # Search for the old felt value echo "Occurrences of the old felt value:" rg --type rust "0x1b39" --glob '!crates/katana/executor/tests/executor.rs' # Search for the new felt value echo "Occurrences of the new felt value:" rg --type rust "0x06b86e40118f29ebe393a75469b4d926c7a44c2e2681b6d319520b7c1156d114" --glob '!crates/katana/executor/tests/executor.rs'Length of output: 620
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2521 +/- ##
==========================================
- Coverage 68.59% 68.59% -0.01%
==========================================
Files 387 387
Lines 49988 49988
==========================================
- Hits 34291 34290 -1
- Misses 15697 15698 +1 ☔ View full report in Codecov by Sentry. |
Description
Updates the ERC20 class entrypoints with snake_case equivalents.
Source cairo
Related issue
Tests
Added to documentation?
Checklist
scripts/prettier.sh
,scripts/rust_fmt.sh
,scripts/cairo_fmt.sh
)scripts/clippy.sh
,scripts/docs.sh
)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores