Conversation
|
""" WalkthroughA new step was added to the GitHub Actions integration test workflow to perform an automated test of transaction submission and balance verification on the local blockchain. This step retrieves account balances, executes a transaction, and checks the results, enhancing the workflow's end-to-end coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions Workflow
participant Chain as Local Blockchain Node
CI->>Chain: Retrieve Bob and Alice addresses
CI->>Chain: Query Bob's initial balance
alt Bob's balance >= 100 stake
CI->>Chain: Submit transaction (Bob -> Alice, 100 stake)
Note right of CI: Capture transaction hash
CI->>Chain: Query Bob's balance after transaction
CI->>CI: Validate balance decreased by 100 stake
CI->>CI: Print success or exit on failure
else Bob's balance < 100 stake
CI->>CI: Exit with error
end
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
""" Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 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 (2)
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 (
|
| echo "Sending 100stake from Bob to Alice..." | ||
| TX_HASH=$(gmd tx bank send bob $(gmd keys show alice -a) 100stake -y --output json | jq -r .txhash) | ||
|
|
||
| # query the transaction (TODO: uncomment after tx indexer is fixed) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/integration_test.yml (1)
149-156: Preserve commented tx-query for future re-enablement
The TODO to uncomment the transaction query after the indexer fix matches the existing guidance in PR #68.
🧹 Nitpick comments (2)
.github/workflows/integration_test.yml (2)
124-125: Clarify step name consistency
The step is named "Test Transaction Submission and Query" but the on-chain transaction query is currently commented out. Consider renaming it to reflect the active functionality (e.g., "Test Transaction Submission and Balance Update") until the query portion is re-enabled.
128-133: Optimize address retrieval and reuse
You fetch both Bob’s and Alice’s addresses here but later invokegmd keys show aliceagain when sending the tx. To avoid redundant calls and improve readability, use the$ALICE_ADDRESSvariable in subsequent commands.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/integration_test.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / Run Unit Tests
- GitHub Check: Test with Rollkit Chain
🔇 Additional comments (5)
.github/workflows/integration_test.yml (5)
126-127: These lines simply change into thegmdirectory; no issues detected.
134-138: Balance query implementation looks good
The use ofgmd query bank balancespiped throughjqcorrectly retrieves the initial stake amount. No issues here.
158-161: Final balance query is correct
Querying Bob's balance post-transaction withjqis consistent with the initial query. No changes necessary.
162-167: Validation logic for expected balance is sound
Subtracting 100 fromINITIAL_BALANCEand comparing strings works here. Optionally, you could useif (( FINAL_BALANCE != EXPECTED_BALANCE )); thenfor an arithmetic test.
169-169: The success echo is clear and needs no modification.
| # send transaction from bob to alice and get tx hash | ||
| echo "Sending 100stake from Bob to Alice..." | ||
| TX_HASH=$(gmd tx bank send bob $(gmd keys show alice -a) 100stake -y --output json | jq -r .txhash) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reuse $ALICE_ADDRESS and ensure block commitment
When sending the transaction, replace the inline key lookup with the saved variable, and add --broadcast-mode=block so the CLI waits for inclusion in a block before returning. For example:
- TX_HASH=$(gmd tx bank send bob $(gmd keys show alice -a) 100stake -y --output json | jq -r .txhash)
+ TX_HASH=$(gmd tx bank send bob $ALICE_ADDRESS 100stake -y --broadcast-mode=block --output json | jq -r .txhash)This guarantees the state is updated before you query the balance.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # send transaction from bob to alice and get tx hash | |
| echo "Sending 100stake from Bob to Alice..." | |
| TX_HASH=$(gmd tx bank send bob $(gmd keys show alice -a) 100stake -y --output json | jq -r .txhash) | |
| # send transaction from bob to alice and get tx hash | |
| echo "Sending 100stake from Bob to Alice..." | |
| TX_HASH=$(gmd tx bank send bob $ALICE_ADDRESS 100stake -y --broadcast-mode=block --output json | jq -r .txhash) |
| # check that bob has funds | ||
| if [ "$INITIAL_BALANCE" == "" ] || [ "$INITIAL_BALANCE" == "null" ] || [ "$INITIAL_BALANCE" -lt 100 ]; then | ||
| echo "Error: Bob's account not properly funded" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve shell conditional for empty/null and numeric checks
Mixing == for string equality and -lt for numeric comparison can be error-prone. Use -z to test for emptiness and a single = for string equality, reserving -lt for numeric cases. For example:
- if [ "$INITIAL_BALANCE" == "" ] || [ "$INITIAL_BALANCE" == "null" ] || [ "$INITIAL_BALANCE" -lt 100 ]; then
+ if [ -z "$INITIAL_BALANCE" ] || [ "$INITIAL_BALANCE" = "null" ] || [ "$INITIAL_BALANCE" -lt 100 ]; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # check that bob has funds | |
| if [ "$INITIAL_BALANCE" == "" ] || [ "$INITIAL_BALANCE" == "null" ] || [ "$INITIAL_BALANCE" -lt 100 ]; then | |
| echo "Error: Bob's account not properly funded" | |
| exit 1 | |
| fi | |
| # check that bob has funds | |
| if [ -z "$INITIAL_BALANCE" ] || [ "$INITIAL_BALANCE" = "null" ] || [ "$INITIAL_BALANCE" -lt 100 ]; then | |
| echo "Error: Bob's account not properly funded" | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
the hard part i have with this flow is it requires installing a new tool to run tests (act), when we want to expand the set of tests, ibc, cosmwasm, rpc compat testing, relayer testing, comet migration, etc.. do we need to write new yaml files for each? i dont understand how to reuse this flow for many other test cases we should do.
i dont think this solves the issue, we should have a cosmwasm test as well.
For more fleshed out tests we should eventually fork system test and use that. This was the fastest to create, as this is basically automatizing the basic local testing and avoid regressions in the short/mid term. Concerning cosmwasm, this can easily be added here or in another workflow (depending on what part of cosmwasm we want to test). We can even split the app scaffolding in multiple actions to get different flavor of an app (app with ibc, app with cosmwasm, ...) Adding cosmwasm should eventually be as simple as: ignite app install github.com/ignite/apps/wasm@main
ignite app install github.com/ignite/apps/rollkit@main
ignite wasm add
ignite rollkit add
ignite rollkit init
ignite wasm config |
* ci: add tx test in integration test * updates * updates
2 participants
ref: #42
Summary by CodeRabbit