fix: constants #305
fix: constants #305
Conversation
WalkthroughThe changes involve updates to constant values in the Changes
Possibly related PRs
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/core/src/constants/index.ts (1 hunks)
- worlds/dojo-starter (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- worlds/dojo-starter
🧰 Additional context used
🔇 Additional comments (3)
packages/core/src/constants/index.ts (3)
4-5: Verify the new KATANA_PREFUNDED_ADDRESS valueThe KATANA_PREFUNDED_ADDRESS has been updated. While the new value appears to be a valid Starknet address, please confirm:
- Is this the correct new address for the Katana prefunded account?
- Have all necessary systems been updated to use this new address?
- Are there any implications for existing tests or development environments?
To check for any hardcoded usage of the old address, run:
8-9: Verify the new KATANA_CLASS_HASH valueThe KATANA_CLASS_HASH has been updated. While the new value appears to be a valid Starknet class hash, please confirm:
- Is this the correct new class hash for the Katana environment?
- What changes in the contract or compilation process led to this new class hash?
- Have all necessary systems been updated to use this new class hash?
- Are there any implications for existing contract deployments or interactions?
To check for any hardcoded usage of the old class hash, run:
4-9: Provide context for the overall changes to Katana constantsThese changes to KATANA_PREFUNDED_ADDRESS, KATANA_PREFUNDED_PRIVATE_KEY, and KATANA_CLASS_HASH represent significant updates to core Katana constants. To ensure a smooth transition and prevent potential issues:
- Please provide context for why these changes were necessary. What issue does this "fix: constants" address?
- Has the Katana documentation been updated to reflect these new values?
- Are there any migration steps required for developers using these constants?
- How do these changes relate to the update in the
worlds/dojo-startersubproject commit (8e84a8b62815bafcac48fcff299db2dd124efb2c)?To check for any documentation or README files that might need updating, run:
✅ Verification successful
[/run_scripts]
[/run_scripts]
Katana constants have been successfully updated and old values removed
The
KATANA_PREFUNDED_ADDRESS,KATANA_PREFUNDED_PRIVATE_KEY, andKATANA_CLASS_HASHconstants have been correctly updated with new values, and all obsolete values have been removed from the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for files that might contain references to Katana constants fd -e md -e txt | xargs rg -i "katana.*address|katana.*key|katana.*hash"Length of output: 74
Script:
#!/bin/bash # Search for references to Katana constants in TypeScript and JavaScript files fd -e ts -e js | xargs rg -i "KATANA_PREFUNDED_ADDRESS|KATANA_PREFUNDED_PRIVATE_KEY|KATANA_CLASS_HASH"Length of output: 1417
Script:
#!/bin/bash # Retrieve git diff for lines 4-9 in packages/core/src/constants/index.ts git diff HEAD~1 HEAD -- packages/core/src/constants/index.ts | sed -n '4,9p'Length of output: 387
packages/core/src/constants/index.ts
Outdated
| export const KATANA_PREFUNDED_PRIVATE_KEY = | ||
| "0x2bbf4f9fd0bbb2e60b0316c1fe0b76cf7a4d0198bd493ced9b8df2a3a24d68a"; | ||
| "0x127fd5f1fe78a71f8bcd1fec63e3fe2f0486b6ecd5c86a0466c3a21fa5cfcec"; |
There was a problem hiding this comment.
Critical: KATANA_PREFUNDED_PRIVATE_KEY matches KATANA_PREFUNDED_ADDRESS
The new value for KATANA_PREFUNDED_PRIVATE_KEY is identical to the new KATANA_PREFUNDED_ADDRESS. This is highly unusual and potentially a security risk. Private keys and addresses should be distinct in cryptographic systems. Please urgently clarify:
- Is this intentional? If so, what's the rationale?
- If not, what should the correct private key be?
- Has this been exposed in any public environments?
If this is unintentional, please update the private key to a distinct, secure value immediately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
readme.md (1)
49-49: LGTM. Consider adding version information.The update to
sozo migrate applyis correct and aligns with the more explicit command syntax. This change improves clarity for users following the quick start guide.Consider adding a note about the minimum required Sozo version for this command, to ensure users have the compatible version installed. For example:
sozo build && sozo migrate apply # Requires Sozo v0.X.X or higherAlso, it might be helpful to update any related documentation or guides that mention the
sozo migratecommand to maintain consistency across the project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- examples/example-vite-react-app-recs/src/App.tsx (1 hunks)
- packages/state/src/recs/index.ts (7 hunks)
- readme.md (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/example-vite-react-app-recs/src/App.tsx
🧰 Additional context used
🔇 Additional comments (6)
packages/state/src/recs/index.ts (6)
54-56: Improved logging in getSyncEntities functionThe addition of the logging statement enhances debugging capabilities by providing more context about the function execution, including the
clauseparameter. This change will make it easier to track and understand the filtering applied during entity synchronization.
279-279: Approved: Minor formatting improvementThe addition of an empty line improves code readability by separating the logging statement from the
setEntitiescall. This change doesn't affect functionality and is a good practice for maintaining clean, well-structured code.
325-334: Improved robustness in setEntities functionThe new conditional logic enhances the function's robustness by handling edge cases:
- When there are no entities to set.
- When there's a single empty entity (with key "0x0").
This change prevents unnecessary processing in these scenarios and provides a helpful warning message. The added empty line also improves code readability by clearly separating this new logic from the rest of the function.
These improvements will help catch potential issues early and make debugging easier.
170-170: Verify impact of including hashed keys in event messagesThe
dont_include_hashed_keysparameter has been set tofalsefor event messages as well. This change is consistent with the modification in thegetEntitiesfunction. While it may provide more detailed information in event messages, it could also increase the data volume. Please ensure that this change is intentional and doesn't negatively impact performance or data processing for event handling.To assess the impact of including hashed keys in event messages, you can run the following script:
This script will help identify all instances where
client.getEventMessagesis called and compare the results with and without hashed keys.
237-245: Verify impact of including hashed keys and improved logging in getEntitiesQuery
The
dont_include_hashed_keysparameter has been set tofalse, consistent with changes in other functions. This change will include hashed keys in the query results, potentially increasing the amount of data returned. Please verify that this change is intentional and doesn't negatively impact query performance or data processing.A new logging statement has been added to provide information about the number of fetched entities and the current cursor position. This improves visibility for debugging and monitoring the query progress. However, consider wrapping this log statement in a condition to only execute when
loggingis true, to avoid unnecessary logging in production environments.To assess the impact of including hashed keys in entity queries, you can run the following script:
This script will help identify all instances where
client.getEntitiesis called in queries and compare the results with and without hashed keys.
129-133: Verify impact of including hashed keys and additional logging
The
dont_include_hashed_keysparameter has been set tofalse. This change will include hashed keys in the retrieved entities, potentially increasing the amount of data returned. Please verify that this change is intentional and doesn't negatively impact performance or data processing.A new logging statement has been added to output the fetched entities. While this improves visibility for debugging, it may significantly increase log verbosity, especially with large datasets. Consider adding a condition to only log this information when
loggingis true.To assess the impact of including hashed keys, you can run the following script:
This script will help identify all instances where
client.getEntitiesis called and compare the results with and without hashed keys.
Summary by CodeRabbit
Bug Fixes
Chores
dojoupcommand version and adding a step to update submodules.Documentation
sozo migratetosozo migrate applyfor clarity.Improvements
useQuerySynchook in the application.