-
Notifications
You must be signed in to change notification settings - Fork 32
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
Update new braavos class_hash and fix calldata in BraavosAccountDeriv… #407
Conversation
To view this pull requests documentation preview, visit the following URL: docs.page/focustree/starknet.dart~407 Documentation is deployed and generated using docs.page. |
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/starknet/lib/src/account.dart (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/starknet/lib/src/account.dart (1)
Line range hint
632-634
: Verify the simplification ofconstructorCalldata
methodThe
constructorCalldata
method has been simplified to only return[publicKey]
. This change might affect the account deployment process.
- Please confirm that this simplification is intentional and aligns with the latest Braavos account specifications.
- Ensure that removing
implementationClassHash
andinitializerSelector
doesn't break any existing functionality or deployments.To verify the impact of this change, please run the following script:
#!/bin/bash # Description: Check for any references to the removed parameters in the codebase # Test: Search for any usage of implementationClassHash in relation to Braavos accounts rg --type dart "implementationClassHash.*Braavos" # Test: Search for any usage of initializerSelector in relation to Braavos accounts rg --type dart "initializerSelector.*Braavos" # Test: Look for any recent changes related to Braavos account initialization git log -n 10 --grep="Braavos" --grep="constructor" --grep="calldata" --pretty=format:"%h - %s" --author-date-order
// FIXME: hardcoded value 2024-10-14 | ||
// As mentioned in Braavos repo https://github.com/myBraavos/braavos-account-cairo | ||
final classHash = Felt.fromHexString( | ||
"0x03131fa018d520a037686ce3efddeab8f28895662f019ca3ca18a626650f7d1e", | ||
); | ||
|
||
/// FIXME: implementation class hash should be retrieved at runtime | ||
final implementationClassHash = Felt.fromHexString( | ||
"0x5aa23d5bb71ddaa783da7ea79d405315bafa7cf0387a74f4593578c3e9e6570", | ||
"0x013bfe114fb1cf405bfc3a7f8dbe2d91db146c17521d40dcf57e16d6b59fa8e6", |
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.
💡 Codebase verification
classHash
Value Correctly Updated but Still Hardcoded
The classHash
has been updated in commit 14f513e
, aligning with the latest Braavos account implementation. However, the value remains hardcoded as indicated by the FIXME comment.
- Action Required: Implement a dynamic retrieval mechanism for
classHash
to replace the hardcoded value.
🔗 Analysis chain
Update of classHash
value needs verification
The classHash
value has been updated, which is likely related to a new implementation of the Braavos account. However, the comment still indicates that this is a hardcoded value.
- Please verify that this new
classHash
value is correct and up-to-date with the latest Braavos account implementation. - Consider implementing a mechanism to retrieve this value at runtime instead of hardcoding it, as suggested by the FIXME comment.
To ensure the classHash
value is correct, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Braavos account class hash
# Test: Check if the new class hash is mentioned in any documentation or recent commits
rg --type md --type git "013bfe114fb1cf405bfc3a7f8dbe2d91db146c17521d40dcf57e16d6b59fa8e6"
# Test: Look for any references to Braavos account updates in recent commits
git log -n 10 --grep="Braavos" --grep="account" --grep="class hash" --pretty=format:"%h - %s" --author-date-order
Length of output: 296
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.
this recommendation is out of scope for this fix
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.
@devnet0x, understood. Thank you for clarifying.
╭( •_•)╮
Update new braavos class_hash and fix calldata in BraavosAccountDerivation. Close issue #393
Summary by CodeRabbit
New Features
Bug Fixes
Documentation