-
Notifications
You must be signed in to change notification settings - Fork 52
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
fix: dojoConfig manifest path for onchain-dash #344
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to import paths and the restructuring of configuration files within various components of the project. Notably, the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1).github/workflows/ci.yaml (1)
The update from v1.0.0-rc.2 to v1.0.2 is a positive change as it moves to a stable version. However, let's verify that this version is compatible with the subsequent build steps. ✅ Verification successfulVersion update is safe to proceed The verification shows that the codebase primarily uses 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify compatibility between dojoup v1.0.2 and the build pipeline
# Check if sozo commands are referenced with specific version requirements
rg -A 2 "sozo (build|test)"
# Look for any version constraints or dependencies in configuration files
fd -e toml -e json -H "manifest|config|package" --exec cat {} \; | rg -i "dojo|sozo|katana"
Length of output: 8241 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
🧹 Outside diff range and nitpick comments (2)
examples/example-vite-kitchen-sink/dojoConfig.ts (1)
Line range hint
3-7
: Consider adding a comment explaining the manifest choiceSince this is an example project using a development manifest, it would be helpful to add a brief comment explaining why this specific manifest is used and what it contains.
+// Using development manifest for example setup import manifest from "../../worlds/onchain-dash/manifest_dev.json"; export const dojoConfig = createDojoConfig({ manifest, });
examples/example-nodejs-bot/package.json (1)
35-35
: LGTM! Consider updating documentationThe simplified output path is cleaner and more maintainable. This change aligns well with standard Node.js project structures.
Consider updating any related documentation or README files that might reference the old build output path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
examples/example-nodejs-bot/package.json
(1 hunks)examples/example-vite-kitchen-sink/dojoConfig.ts
(1 hunks)examples/example-vite-kitchen-sink/src/components/caller-counter.tsx
(1 hunks)examples/example-vite-kitchen-sink/src/components/global-counter.tsx
(1 hunks)examples/example-vite-kitchen-sink/src/components/starknet-provider.tsx
(1 hunks)examples/example-vite-kitchen-sink/src/components/theme-switch.tsx
(1 hunks)examples/example-vite-kitchen-sink/src/dojo.config.ts
(0 hunks)worlds/dojo-starter
(1 hunks)worlds/onchain-dash
(1 hunks)
💤 Files with no reviewable changes (1)
- examples/example-vite-kitchen-sink/src/dojo.config.ts
✅ Files skipped from review due to trivial changes (5)
- examples/example-vite-kitchen-sink/src/components/caller-counter.tsx
- examples/example-vite-kitchen-sink/src/components/global-counter.tsx
- examples/example-vite-kitchen-sink/src/components/theme-switch.tsx
- worlds/dojo-starter
- worlds/onchain-dash
🔇 Additional comments (4)
examples/example-nodejs-bot/package.json (1)
35-35
: Verify TypeScript output directory configuration
The serve script now uses a simplified output path dist/src/index.js
. Please ensure this aligns with the TypeScript configuration.
✅ Verification successful
TypeScript configuration aligns with the serve script path
The TypeScript configuration is correctly set with "outDir": "dist"
and includes "./src/**/*"
in its sources, which perfectly aligns with the serve script's path dist/src/index.js
. The source files structure also confirms this with src/index.ts
that will compile to the expected location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check TypeScript configuration for output directory settings
# Look for tsconfig.json and its outDir setting
fd tsconfig.json --exec cat {} | jq -r 'select(.compilerOptions.outDir != null) | .compilerOptions.outDir'
# Verify the existence of the new output path
fd index.js -p dist/src/ || echo "Warning: Output path not found"
Length of output: 363
Script:
#!/bin/bash
# Let's first check the content of tsconfig.json directly
cat examples/example-nodejs-bot/tsconfig.json
# Also check if there are any TypeScript files that might indicate the source structure
fd -e ts . examples/example-nodejs-bot/
Length of output: 1143
examples/example-vite-kitchen-sink/src/components/starknet-provider.tsx (3)
4-4
: Verify wallet connectivity requirements
The removal of argent
and braavos
imports from @starknet-react/core suggests that these wallet connectors are no longer needed. Please confirm that CartridgeConnector is sufficient for your authentication needs.
✅ Verification successful
Removal of wallet connectors is intentional and correct
The codebase has been intentionally migrated to use CartridgeConnector exclusively, as evidenced by:
- The StarknetConfig component is properly configured with only CartridgeConnector
- The package.json shows @cartridge/connector as a dependency
- The only remaining references to argent/braavos are in comments and documentation, not in functional code
- The README acknowledges this change by mentioning that argent & braavos wallets require manual setup if needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any remaining references to argent or braavos wallets
# that might be affected by this change
rg -i "argent|braavos" --type tsx --type ts
Length of output: 73
Script:
#!/bin/bash
# Check for any references to argent or braavos wallets in all files
# Using ripgrep without file type restrictions to ensure we catch all occurrences
rg -i "argent|braavos"
# Also check the package.json for any wallet-related dependencies
cat examples/example-vite-kitchen-sink/package.json
# Look for any StarknetConfig usage to understand the wallet configuration
rg "StarknetConfig" -A 5 -B 5
Length of output: 5059
6-6
: Verify dojoConfig accessibility
The import path for dojoConfig has been changed to use a parent directory reference (@/../dojoConfig
). Let's verify this configuration file exists and is properly referenced.
✅ Verification successful
Import path is valid and consistent with project structure
The dojoConfig
import path @/../dojoConfig
is valid and properly referenced. The file exists at examples/example-vite-kitchen-sink/dojoConfig.ts
and is being used consistently across multiple components in the same project. The @
alias appears to be a valid path alias in the Vite project, and the relative path navigation to the config file is working as intended.
- The config file is properly structured and exports the
dojoConfig
object - Multiple components in the same project use identical import paths
- The config is actively used in the StarknetProvider component for contract configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if dojoConfig exists in the expected location and
# find all files that depend on this configuration
echo "Checking for dojoConfig file..."
fd dojoConfig
echo "Checking for all imports of dojoConfig..."
rg -l "from.*dojoConfig" --type tsx --type ts
Length of output: 750
Script:
#!/bin/bash
# Let's try a different approach to find imports
echo "Checking for imports of dojoConfig..."
rg "from.*dojoConfig"
# Also check the specific file content
echo -e "\nChecking the specific file content..."
cat examples/example-vite-kitchen-sink/src/components/starknet-provider.tsx
# And verify the relative path resolution
echo -e "\nChecking dojoConfig content in the example..."
cat examples/example-vite-kitchen-sink/dojoConfig.ts
Length of output: 3658
Line range hint 9-27
: Verify contract address references
The CartridgeConnector policies are using contract addresses from dojoConfig.manifest. Since this PR involves path changes, we should ensure these contract references are still valid.
@@ -1,6 +1,6 @@ | |||
import { createDojoConfig } from "@dojoengine/core"; | |||
|
|||
import manifest from "../../worlds/onchain-dash/manifests/release/deployment/manifest.json"; | |||
import manifest from "../../worlds/onchain-dash/manifest_dev.json"; |
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
Path inconsistency detected in manifest import
The manifest path in examples/example-vite-kitchen-sink/dojoConfig.ts
is inconsistent with other examples:
- All other examples use
../../worlds/dojo-starter/manifest_dev.json
- This file uniquely uses
../../worlds/onchain-dash/manifest_dev.json
which doesn't exist - The path should be aligned with the standard pattern used across all other example projects
🔗 Analysis chain
Verify the manifest path exists and is consistent
The change from release manifest to development manifest makes sense for an example project, but let's verify the path exists and is consistent with other examples.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify manifest path existence and consistency across examples
# Check if the new manifest path exists
echo "Checking if manifest_dev.json exists:"
ls -l worlds/onchain-dash/manifest_dev.json
# Check for similar manifest usage patterns in other examples
echo "\nChecking manifest usage patterns in other examples:"
rg -l "manifest.*\.json" examples/
Length of output: 904
Script:
#!/bin/bash
# Let's check the manifest paths used in other example dojoConfig files
echo "Checking manifest paths in other example dojoConfig files:"
rg "manifest.*\.json" examples/*/dojoConfig.ts
Length of output: 1840
Updates worlds version + update path for examples
Summary by CodeRabbit
New Features
torii-bot
project to simplify script execution.manifest
in the Dojo configuration, enhancing development flexibility.Bug Fixes
dojoConfig
in various components to ensure proper configuration access.Chores
dojo.config.ts
file to streamline the codebase.dojoup
command for better performance.