Increase player's success chances #65
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Walkthrough
A new constant for move success probability was introduced, and the move reveal logic was updated to use this probability in determining move outcomes, replacing the previous deterministic direction-based approach. Minor code cleanup and comment updates were also made, without altering any public interfaces.
## Changes
| File(s) | Change Summary |
|------------------------------------------------------------------|--------------------------------------------------------------------------------------------------|
| programs/blockrunners/src/constants.rs | Added `MOVE_SUCCESS_PROBABILITY` constant with documentation and attribute. |
| programs/blockrunners/src/instructions/move_reveal.rs | Refactored move reveal logic to use probabilistic success based on `MOVE_SUCCESS_PROBABILITY`; renamed event message variables; reworded event messages. |
| programs/blockrunners/src/instructions/initialize_player.rs | Removed unused `game_state` variable from `initialize_player` function. |
| programs/blockrunners/src/utils/randomness_reveal.rs | Updated test-mode comment to clarify always-successful move and card outcome; no code changes. |
| app/src/idl/blockrunners.json | Added `MOVE_SUCCESS_PROBABILITY` constant with documentation. |
| app/src/idl/blockrunners.ts | Added `moveSuccessProbability` constant with documentation; added `debugGiveCard` instruction. |
| app/src/lib/utils.ts | Removed `getMsgLogs` function. |
| programs/blockrunners/src/instructions/join_game.rs | Refactored join message construction into a local variable before event emission. |
| programs/blockrunners/src/instructions/purchase_ciphers.rs | Created local message variable for purchase success; removed debug log. |
| programs/blockrunners/src/utils/give_random_cards.rs | Created local message variable for max cards collected event emission. |
| tests/helpers/utils.ts | Split `getMsgLogs` into `getTxDetails` and updated `getMsgLogs` to accept tx details; added `getEventLogs` for event parsing. |
| tests/initialize_game.ts | Updated tests to use `getTxDetails` before `getMsgLogs` and added `getEventLogs` to log events. |
| tests/initialize_player.ts | Updated tests to use `getTxDetails` before `getMsgLogs` and added `getEventLogs` to log events. |
| tests/join_game.ts | Updated tests to use `getTxDetails` before `getMsgLogs` and added `getEventLogs` to log events. |
| tests/move.ts | Updated tests to use `getTxDetails` before `getMsgLogs` and added `getEventLogs`; removed one test case and some debug logs. |
| tests/purchase_ciphers.ts | Updated tests to use `getTxDetails` before `getMsgLogs` and added `getEventLogs`; updated one assertion. |
| Anchor.toml | Changed provider cluster from "devnet" to "localnet". |
| README.md | Removed "TODO:" prefix from a sentence about default connection in README. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Player
participant MoveReveal
participant Randomness
participant GameState
Player->>MoveReveal: Submit move reveal
MoveReveal->>Randomness: Generate randomness value
Randomness-->>MoveReveal: Return value
MoveReveal->>MoveReveal: Compute (randomness % 100 < MOVE_SUCCESS_PROBABILITY)
alt Success
MoveReveal->>GameState: handle_correct_move
else Failure
MoveReveal->>GameState: handle_incorrect_move
end
MoveReveal->>Player: Reset move and cards commitmentAssessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Suggested reviewers
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
programs/blockrunners/src/utils/randomness_reveal.rs (1)
23-23: 💡 Verification agent🧩 Analysis chain
Verify missing import for RandomnessAccountData.
The code uses
RandomnessAccountData::parse()but I don't see the corresponding import statement in the provided code. The AI summary mentions this import was removed, but the type is still being used.
🏁 Script executed:
#!/bin/bash # Check if RandomnessAccountData is imported or re-exported from another module rg -A 3 -B 3 "RandomnessAccountData" --type rustLength of output: 2654
🏁 Script executed:
#!/bin/bash rg "^use " programs/blockrunners/src/utils/randomness_reveal.rsLength of output: 154
Add missing
RandomnessAccountDataimportThe file
programs/blockrunners/src/utils/randomness_reveal.rscallsRandomnessAccountData::parse(...)but doesn’t import that type. Please add:use crate::{errors::BlockrunnersError, state::PlayerState}; +use switchboard_on_demand::RandomnessAccountData;
🧹 Nitpick comments (1)
programs/blockrunners/src/instructions/move_reveal.rs (1)
92-104: Solid probabilistic logic implementation with minor suggestions.The new probabilistic approach correctly implements a 75% success rate and significantly improves the game mechanics by making success more likely while still maintaining some challenge.
The mathematical logic
(random_value % 100) < MOVE_SUCCESS_PROBABILITYis correct and will give exactly the desired success probability.Consider these improvements:
- Extract magic number as constant:
// In constants.rs +#[constant] +pub const PERCENTAGE_BASE: u8 = 100; // In move_reveal.rs imports - constants::{GAME_STATE_SEED, MOVE_SUCCESS_PROBABILITY, PLAYER_STATE_SEED}, + constants::{GAME_STATE_SEED, MOVE_SUCCESS_PROBABILITY, PERCENTAGE_BASE, PLAYER_STATE_SEED}, // In the logic - let is_move_successful = (random_value % 100) < MOVE_SUCCESS_PROBABILITY; + let is_move_successful = (random_value % PERCENTAGE_BASE) < MOVE_SUCCESS_PROBABILITY;
- Add validation for MOVE_SUCCESS_PROBABILITY:
+ // Ensure the probability is valid (should be <= 100) + debug_assert!(MOVE_SUCCESS_PROBABILITY <= 100, "Move success probability must be <= 100");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
programs/blockrunners/src/constants.rs(1 hunks)programs/blockrunners/src/instructions/move_reveal.rs(2 hunks)programs/blockrunners/src/utils/randomness_reveal.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
programs/blockrunners/src/constants.rs (1)
33-35: LGTM! Well-documented constant addition.The new
MOVE_SUCCESS_PROBABILITYconstant is properly documented and follows the established patterns. The 75% success rate aligns with the PR objective of increasing player success chances.programs/blockrunners/src/utils/randomness_reveal.rs (1)
70-70: Good comment update to reflect new logic.The comment correctly explains that moves are always successful in test mode, which aligns with the new probabilistic approach.
programs/blockrunners/src/instructions/move_reveal.rs (1)
4-4: Good addition of required import.The import of
MOVE_SUCCESS_PROBABILITYis necessary for the new probabilistic logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
tests/helpers/utils.ts (2)
72-95:confirmTransactioncall may mis-confirm & missing null-guard
confirmTransactionis invoked with a freshlatestBlockhash. If the target signature was produced in an earlier slot, the hash/height pair will not match and the confirmation can silently fail.
Use the simpler overload that takes only the signature (or fetch its block-hash fromgetTransaction).
txDetailscan benull(RPC nodes may prune old data). Subsequent callers assume a non-null object, leading toCannot read properties of null (reading 'meta').-const latestBlockHash = await provider.connection.getLatestBlockhash(); -await provider.connection.confirmTransaction( - { - blockhash: latestBlockHash.blockhash, - lastValidBlockHeight: latestBlockHash.lastValidBlockHeight, - signature: txSignature, - }, - "confirmed" -); +await provider.connection.confirmTransaction(txSignature, "confirmed"); ... -const txDetails = await provider.connection.getTransaction(txSignature, { +const txDetails = await provider.connection.getTransaction(txSignature, { maxSupportedTransactionVersion: 0, commitment: "confirmed", }); -if (!txDetails) return null; +if (!txDetails) return null;
97-116:getMsgLogs— stale JSDoc & unsafe access
- The comment still mentions
provider/signature; the function now receivestxDetails.- Accessing
txDetails.meta.logMessageswithout a null-check will crash whentxDetailsisnull.- Returning
nullwhen there are simply no logs forces all callers to branch; returning an empty array is simpler.- * Get logs from program's msg! macro obtained from a transaction - * @param provider The Anchor provider - * @param signature The transaction signature + * Extract `msg!` logs from an already-fetched transaction + * @param txDetails The versioned transaction object ... -export const getMsgLogs = async ( - txDetails: anchor.web3.VersionedTransactionResponse -): Promise<string[] | null> => { +export const getMsgLogs = ( + txDetails: anchor.web3.VersionedTransactionResponse | null +): string[] => { try { - const logs = txDetails?.meta?.logMessages + if (!txDetails?.meta?.logMessages) return []; + const logs = txDetails.meta.logMessages ?.filter((log) => log.includes("Program log:")) .map((log) => log.replace("Program log: ", "")); - return logs || null; + return logs ?? []; } catch (error) { console.error("Failed to get transaction logs:", error); - return null; + return []; } };
🧹 Nitpick comments (3)
tests/helpers/utils.ts (3)
2-4: Prefer one consistentProgramimport styleYou import
Programdirectly ({ ... , Program }) and later still referenceanchor.Program. Importing both names is redundant and may confuse new contributors. Either drop the explicitProgramimport or replace allanchor.Programusages with the imported symbol, but don’t keep both.-import { BorshCoder, EventParser, Program } from "@coral-xyz/anchor"; +import { BorshCoder, EventParser } from "@coral-xyz/anchor";
67-71: JSDoc drift — parameters no longer match implementation
getTxDetailsnow receives onlyproviderandtxSignature, yet the JSDoc still documents provider / signature in a style left over from the old API. Keep the comments in sync to avoid stale documentation.
118-140:getEventLogs— same doc mismatch & possible undefined accessMirror the fixes applied to
getMsgLogs:
- Update JSDoc to match the parameter.
- Guard against
nullor missingmeta.logMessages.event.data?.messagemay beundefined; consider JSON-stringifying the whole event or omitting the message when absent to avoid"undefined"strings in test output.- * Get logs from program's msg! macro obtained from a transaction - * @param provider The Anchor provider - * @param signature The transaction signature + * Parse Anchor events from a fetched transaction + * @param txDetails The versioned transaction object ... -export const getEventLogs = async ( - txDetails: anchor.web3.VersionedTransactionResponse -): Promise<string[] | null> => { +export const getEventLogs = ( + txDetails: anchor.web3.VersionedTransactionResponse | null +): string[] => { try { - const eventParser = new EventParser(program.programId, new BorshCoder(program.idl)); - const events = eventParser.parseLogs(txDetails.meta.logMessages); + if (!txDetails?.meta?.logMessages) return []; + const parser = new EventParser(program.programId, new BorshCoder(program.idl)); + const events = parser.parseLogs(txDetails.meta.logMessages); const eventLogs: string[] = []; - for (let event of events) { - eventLogs.push(`${event.name}: ${event.data?.message}`); + for (const event of events) { + eventLogs.push( + event.data?.message !== undefined + ? `${event.name}: ${event.data.message}` + : event.name + ); } - return eventLogs || null; + return eventLogs; } catch (error) { console.error("Failed to get transaction logs:", error); - return null; + return []; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/src/idl/blockrunners.json(1 hunks)app/src/idl/blockrunners.ts(1 hunks)app/src/lib/utils.ts(0 hunks)programs/blockrunners/src/instructions/join_game.rs(1 hunks)programs/blockrunners/src/instructions/move_reveal.rs(10 hunks)programs/blockrunners/src/instructions/purchase_ciphers.rs(1 hunks)programs/blockrunners/src/utils/give_random_cards.rs(1 hunks)tests/helpers/utils.ts(4 hunks)tests/initialize_game.ts(2 hunks)tests/initialize_player.ts(2 hunks)tests/join_game.ts(2 hunks)tests/move.ts(5 hunks)tests/purchase_ciphers.ts(5 hunks)
💤 Files with no reviewable changes (1)
- app/src/lib/utils.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/join_game.ts
- tests/initialize_game.ts
- programs/blockrunners/src/utils/give_random_cards.rs
- tests/move.ts
- programs/blockrunners/src/instructions/join_game.rs
- app/src/idl/blockrunners.json
- tests/initialize_player.ts
- programs/blockrunners/src/instructions/move_reveal.rs
- app/src/idl/blockrunners.ts
- tests/purchase_ciphers.ts
🧰 Additional context used
🧠 Learnings (1)
tests/helpers/utils.ts (1)
Learnt from: jannden
PR: jannden/blockrunners#60
File: app/src/hooks/useProgram.tsx:82-82
Timestamp: 2025-04-22T10:21:47.227Z
Learning: The `@coral-xyz/anchor` library at version ^0.30.1 supports initializing a `Program` instance with just two arguments: `new Program(idl, provider)` although TypeScript may show type errors requiring the `@ts-expect-error` comment.
🧬 Code Graph Analysis (1)
programs/blockrunners/src/instructions/purchase_ciphers.rs (1)
programs/blockrunners/src/utils/save_and_emit_event.rs (1)
save_and_emit_event(8-32)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
jannden
force-pushed
the
jannden/feats
branch
3 times, most recently
from
8d2f957 to
a0956ed
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: Increase player's success chances, also closes #30
Summary by CodeRabbit