-
Notifications
You must be signed in to change notification settings - Fork 57
Update gm-world tutorial #575
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
Conversation
WalkthroughThe installation script for the local DA network was updated to change the working directory after cloning and to remove the branch checkout step. The gm-world tutorial was modernized to use current Ignite and Rollkit commands, clarify setup steps, update example outputs, and rename account variables for improved clarity. The comprehensive Ignite Rollkit guide was deleted. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InstallScript
participant RollkitRepo
User->>InstallScript: Run install-local-da.sh
InstallScript->>RollkitRepo: git clone --depth=1
InstallScript->>InstallScript: cd rollkit
InstallScript->>InstallScript: make build-da
InstallScript->>InstallScript: ./build/da-mock
sequenceDiagram
participant User
participant IgniteCLI
participant Rollkit
participant Blockchain
User->>IgniteCLI: ignite scaffold chain
User->>Rollkit: ignite rollkit add
User->>Blockchain: ignite chain build --skip-proto
User->>Rollkit: ignite rollkit init
Rollkit->>User: Initialize config, create alice & bob accounts
User->>Blockchain: Start node, send transactions (ALICE, BOB)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
Summary of Changes
Hello @pthmas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly improves the clarity and completeness of the gm-world
tutorial, making it more accessible for users with limited prior knowledge of the project. The changes streamline setup processes, update command syntax, and provide better contextual explanations, ensuring a smoother learning experience.
Highlights
- Tutorial Clarity & Completeness: The
gm-world
tutorial has been updated with additional, crucial steps for setting up the Rollkit integration and initializing the chain, along with clearer explanations for beginners. - Command Syntax Updates: The
ignite
command for scaffolding a chain has been updated fromignite s chain
toignite scaffold chain
to reflect current CLI usage, ensuring the tutorial remains accurate. - Improved Scripting Logic: The
install-local-da.sh
script was refined to correctly navigate the cloned repository and remove an unnecessarygit checkout
step, streamlining the local DA setup process. - Enhanced Readability: Generic key names (
KEY1
,KEY2
) in the tutorial's transaction examples have been replaced with more descriptive names (ALICE
,BOB
) for better understanding and ease of use. - Documentation Alignment: Expected console outputs and installation commands for the local DA script have been updated to match the current behavior of the underlying systems, preventing user confusion.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
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.
Code Review
The pull request updates the gm-world tutorial to be more accessible to users with less prior knowledge. Changes include using ignite scaffold
instead of ignite s chain
, adding steps to add Rollkit features, build the chain, and initialize it, and updating the commands and log outputs. Variable names have also been updated for clarity. The removal of the tag in the install-local-da script and the TODO
statement should be addressed.
07769de
to
59aee15
Compare
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: 0
♻️ Duplicate comments (1)
tutorials/gm-world.md (1)
94-95
: Unpinned DA install script may break in the future
Previous feedback already raised this; reiterating for emphasis. If the script changes incompatibly, newcomers will be stuck. Pinning a tag or commit keeps the tutorial deterministic.
🧹 Nitpick comments (5)
tutorials/gm-world.md (5)
62-66
: Wording doesn’t match the command – no version is pinned
The sentence says “Install a specific version of ignite to use rollkit”, yet the command installs the plugin without a version constraint:ignite app install -g github.com/ignite/apps/rollkitEither pin a known-good tag/commit or rephrase the sentence to avoid implying version pinning.
74-80
: Grammar & clarity tweak for the build/initialise step-Then add it to your blockchain and build your chain and initialise it +Then add Rollkit to your blockchain, build the chain, and initialize it:This removes the British “initialise→initialize” mismatch (flagged by LanguageTool) and makes the sentence read more smoothly.
82-85
: Minor style – numbers below ten should be spelled out
Consider “It will also initialize two accounts,alice
andbob
.” for consistency with the rest of the doc.
114-145
: Verbose log snapshot is brittle
Hard-coded timestamps, IPs and hashes will diverge quickly. Consider truncating to the first few key lines or replacing with an ellipsis (…
) to avoid future mismatches that confuse readers.
182-183
: Command is clear but sample flags could overwhelm beginners
A brief note explaining--fees
and--keyring-backend test
(or a link to docs) would help absolute newcomers understand why they’re required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/install-local-da.sh
(1 hunks)tutorials/gm-world.md
(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/install-local-da.sh
🧰 Additional context used
🧠 Learnings (1)
tutorials/gm-world.md (5)
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:49-69
Timestamp: 2024-10-08T18:35:32.960Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:0-0
Timestamp: 2024-10-20T19:01:50.319Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:0-0
Timestamp: 2024-08-27T02:53:35.650Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:0-0
Timestamp: 2024-10-08T18:35:32.960Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:49-69
Timestamp: 2024-08-27T02:42:42.659Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
🪛 LanguageTool
tutorials/gm-world.md
[uncategorized] ~84-~84: Do not mix variants of the same word (‘initialize’ and ‘initialise’) within a single text.
Context: ...ary files to run a chain. It will also initialize 2 accounts alice
and bob
: ## 🚀 S...
(EN_WORD_COHERENCY)
[uncategorized] ~84-~84: Possible missing comma found.
Context: ...run a chain. It will also initialize 2 accounts alice
and bob
: ## 🚀 Starting you...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~91-~91: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...y running the following command: First lets start the local DA network: ```bash cu...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
[uncategorized] ~179-~179: Possible missing article found.
Context: ... flags, for now, we just want to submit transaction from a high-level perspective): ```bas...
(AI_HYDRA_LEO_MISSING_THE)
🔇 Additional comments (4)
tutorials/gm-world.md (4)
58-60
:ignite scaffold chain
update looks correct
Switching from the deprecatedignite s chain
toignite scaffold chain
aligns the tutorial with Ignite v28+ syntax.
175-176
: Use ofALICE
/BOB
variables greatly improves readability
Nice follow-up to the earlier review suggestion.
225-226
: Query command reads well
No issues; consistent with variable names introduced above.
242-243
: Consistency preserved for sender balance query
Looks good.
59aee15
to
016fbda
Compare
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
♻️ Duplicate comments (1)
tutorials/gm-world.md (1)
118-119
: Un-tagged Local-DA install repeats a previously-flagged riskRemoving
{{constants.localDALatestTag}}
means the script now tracks the head ofmain
, which can change without notice.
See the earlier review at L90 for the same concern.Please reinstate a fixed tag/commit or provide guidance for troubleshooting version drift.
🧹 Nitpick comments (3)
tutorials/gm-world.md (3)
116-116
: Typo: add apostrophe and comma“First lets start” → “First, let’s start”.
-First lets start the local DA network: +First, let’s start the local DA network:
100-100
: Missing comma for readabilityAdd a comma after “accounts”:
-It will also initialize 2 accounts `alice` and `bob`: +It will also initialize 2 accounts, `alice` and `bob`:
204-208
: Grammar: missing articleInsert “a” before “transaction”:
-... we just want to submit transaction from a high-level perspective): +... we just want to submit a transaction from a high-level perspective):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
guides/ignite-rollkit.md
(0 hunks)public/install-local-da.sh
(1 hunks)tutorials/gm-world.md
(6 hunks)
💤 Files with no reviewable changes (1)
- guides/ignite-rollkit.md
🚧 Files skipped from review as they are similar to previous changes (1)
- public/install-local-da.sh
🧰 Additional context used
🧠 Learnings (1)
tutorials/gm-world.md (5)
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:49-69
Timestamp: 2024-10-08T18:35:32.960Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:0-0
Timestamp: 2024-10-20T19:01:50.319Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:0-0
Timestamp: 2024-08-27T02:53:35.650Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:0-0
Timestamp: 2024-10-08T18:35:32.960Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:49-69
Timestamp: 2024-08-27T02:42:42.659Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
🪛 LanguageTool
tutorials/gm-world.md
[uncategorized] ~100-~100: Possible missing comma found.
Context: ...kit init ``` It will also initialize 2 accounts alice
and `bob`: ### Initialize Roll...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~116-~116: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...y running the following command: First lets start the local DA network: ```bash cu...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
[uncategorized] ~204-~204: Possible missing article found.
Context: ... flags, for now, we just want to submit transaction from a high-level perspective): ```bas...
(AI_HYDRA_LEO_MISSING_THE)
tutorials/gm-world.md
Outdated
|
||
It will also initialize 2 accounts `alice` and `bob`: | ||
|
||
### Initialize Rollkit CLI Configuration {#initialize-rollkit-cli-configuration} |
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.
Is this section still relevant? There is not rollkit binary to use.
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.
Yes you are right I missed that in my copy paste. Will remove it now
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: 0
♻️ Duplicate comments (3)
tutorials/gm-world.md (3)
109-109
: Still unpinned Local-DA installerRemoving the
{{constants.localDALatestTag}}
argument means the script fetches the latest commit, which may introduce incompatibilities. Pin a tag/commit as suggested earlier.-curl -sSL https://rollkit.dev/install-local-da.sh | bash +curl -sSL https://rollkit.dev/install-local-da.sh | bash -s {{constants.localDALatestTag}}
129-156
: Sample log-output will drift over timeHard-coded timestamps & hashes quickly go stale and confuse readers. Consider trimming to the first few meaningful lines or note that output will differ.
62-66
: Pin Rollkit app version and fix misleading headingThe heading says “Install a specific version of ignite” but the command installs the Rollkit Ignite app and still pulls the latest
main
, risking future breakage (see earlier reviews).-Install a specific version of ignite to use rollkit +Install the Rollkit Ignite app (pin a tested version for stability) -ignite app install -g github.com/ignite/apps/rollkit +# replace v0.3.2 with the latest verified tag +ignite app install -g github.com/ignite/apps/[email protected]
🧹 Nitpick comments (2)
tutorials/gm-world.md (2)
70-81
: Re-order build steps to avoid redundant compilation
make install
compiles the chain. Running it beforeignite rollkit add
means you compile twice.
Move the install step after adding Rollkit features, or drop it and rely onignite chain build
.Example:
-Install your app locally: - -```bash -make install -``` - ## Add Rollkit Features {#add-rollkit-features} ... ignite rollkit add + +# Build & install the modified chain +make install
96-100
: Minor grammar tweak-It will also initialize 2 accounts `alice` and `bob`: +It will also initialize two accounts, `alice` and `bob`:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tutorials/gm-world.md
(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
tutorials/gm-world.md (5)
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:49-69
Timestamp: 2024-10-08T18:35:32.960Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:0-0
Timestamp: 2024-10-20T19:01:50.319Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:0-0
Timestamp: 2024-08-27T02:53:35.650Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:0-0
Timestamp: 2024-10-08T18:35:32.960Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
Learnt from: dumbeng
PR: rollkit/docs#436
File: tutorials/artela-evm-plus-plus.md:49-69
Timestamp: 2024-08-27T02:42:42.659Z
Learning: Even testing private keys should be handled carefully in documentation to avoid promoting bad practices. It's better to use placeholders or provide instructions to generate new keys.
🪛 LanguageTool
tutorials/gm-world.md
[uncategorized] ~100-~100: Possible missing comma found.
Context: ...kit init ``` It will also initialize 2 accounts alice
and `bob`: ## 🚀 Starting your...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~106-~106: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...y running the following command: First lets start the local DA network: ```bash cu...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
[uncategorized] ~194-~194: Possible missing article found.
Context: ... flags, for now, we just want to submit transaction from a high-level perspective): ```bas...
(AI_HYDRA_LEO_MISSING_THE)
🔇 Additional comments (6)
tutorials/gm-world.md (6)
58-60
: ✅ignite scaffold chain
update looks goodSwitching to
ignite scaffold chain gm --address-prefix gm
modernises the tutorial and removes the deprecatedignite s chain
flag.
84-90
: Verify.gm
directory claim
ignite chain build
normally only compiles the binary; home directories are created byignite chain init
/ignite rollkit init
. Ensure this sentence is accurate, otherwise newcomers will look for a non-existent folder.
190-192
: Readable alias variables – nice improvementSwitching to
$ALICE
and$BOB
greatly clarifies the scripts.
197-197
: Transaction example looks correctArguments reflect the new aliases and keep the tutorial coherent.
240-240
: Balance query for$ALICE
aligns with earlier changesNo issues spotted.
257-257
: Balance query for$BOB
aligns with earlier changesNo issues spotted.
public/install-local-da.sh
Outdated
@@ -2,9 +2,7 @@ | |||
|
|||
echo "Downloading Local-DA source code..." | |||
git clone https://github.com/rollkit/rollkit.git |
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.
nit --depth=1
we should add a todo do just do a go install
when the repo is tagged.
2272aed
to
6b75591
Compare
Overview
How to Use Ignite to Create a Rollkit App
as it is now a duplicate of the tutorial.Summary by CodeRabbit
Documentation
Chores