Add High roller gamble (TMB Gambles)#6732
Open
TastyPumPum wants to merge 9 commits intooldschoolgg:masterfrom
Open
Add High roller gamble (TMB Gambles)#6732TastyPumPum wants to merge 9 commits intooldschoolgg:masterfrom
TastyPumPum wants to merge 9 commits intooldschoolgg:masterfrom
Conversation
Ensure CanvasModule is initialized before use.
…into high-roller-gamble
Contributor Author
| @ellipsis-dev review this pr |
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to c51b267 in 2 minutes and 53 seconds. Click for details.
- Reviewed
869lines of code in6files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/mahoji/lib/abstracted_commands/highRollerCommand.ts:69
- Draft comment:
generateUniqueRolls re-rolls duplicate values using a safety counter. Consider potential performance issues if duplicates are frequent. Optimizing the uniqueness strategy may be beneficial. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% The comment is speculative in nature - it says "consider potential performance issues" and "may be beneficial" rather than identifying a concrete problem. The rules explicitly state "Do NOT make speculative comments, such as 'If X, then Y is an issue'. Only comment if it is definitely an issue." The function has a safety counter that throws an error after 1000 attempts, which is a reasonable safeguard. Without evidence that this is actually causing performance problems in practice, this is just a theoretical concern. The comment doesn't provide specific actionable guidance on what to change or how to optimize it. The function does have a safety mechanism in place (throws after 1000 attempts), so it's not completely unprotected. However, if duplicates are very frequent (e.g., with a small pool of items with the same price), this could theoretically be a real issue. The comment might be highlighting a legitimate concern about the algorithm's efficiency. While the concern could be theoretically valid, the comment is still speculative ("consider potential performance issues", "may be beneficial") without demonstrating that this is actually a problem. The rules are clear that speculative comments should be removed. If there was evidence of actual performance issues or a specific scenario where this fails, that would be different. The safety counter of 1000 attempts is reasonable for the max 50 participants. This comment should be deleted because it's speculative and doesn't identify a definite issue. It uses language like "consider" and "may be beneficial" which indicates it's not pointing to a concrete problem requiring a code change. The function has appropriate safeguards in place.
2. src/mahoji/lib/abstracted_commands/highRollerCommand.ts:388
- Draft comment:
In payoutWinners, economy transactions are performed sequentially. Consider wrapping these updates in a database transaction to ensure atomicity and avoid inconsistent state on failure. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is suggesting a code quality improvement around atomicity. However, there are several issues with this comment: 1. It's speculative - it says "consider" which is a soft suggestion 2. The actual balance changes happen viaaddItemsToBankwhich may already have transaction handling 3. TheeconomyTransactionrecords appear to be audit logs, not the actual source of truth for balances 4. The comment doesn't provide strong evidence that there's an actual problem 5. Without seeing the implementation ofaddItemsToBankand understanding the data model, it's hard to know if this is a real issue 6. The GP was already removed from participants before this function, so there's already potential for inconsistency that this wouldn't fix I might be missing that this is a legitimate concern about data consistency in a financial system where atomicity is critical. The comment could be pointing out a real bug where if the process fails partway through the payout loop, some winners get paid and others don't, creating an inconsistent state. While atomicity is important in financial systems, this comment is too speculative and doesn't demonstrate strong evidence of a problem. The comment uses "consider" which makes it a suggestion rather than identifying a definite issue. Additionally, without seeing the full context of howaddItemsToBankworks and whether it has its own transaction handling, this comment requires too much external context to validate. The rules state I should only keep comments with STRONG EVIDENCE that they are correct. This comment should be deleted. It's a speculative suggestion ("consider") about adding database transactions without demonstrating that there's actually a problem. Understanding whether this is needed requires knowledge of theaddItemsToBankimplementation and the broader transaction handling strategy, which is beyond the scope of this file review.
3. src/mahoji/lib/abstracted_commands/highRollerCommand.ts:579
- Draft comment:
The final response is sent via safeEdit() and then returned with interaction.returnStringOrFile(). Ensure that this double messaging pattern is intended and doesn't result in duplicate messages. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%The comment is asking the author to ensure that a double messaging pattern is intended and doesn't result in duplicate messages. This is a request for confirmation of intention, which violates the rules. However, it does point out a potential issue with the code logic, which could be useful for the author to consider. The comment could be rephrased to suggest reviewing the logic for potential duplication without asking for confirmation.
4. src/mahoji/commands/gamble.ts:218
- Draft comment:
High Roller Pot subcommand integration looks correct. Ensure that all validations (e.g. stake and invites) are properly handled in highRollerCommand. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that validations are properly handled, which falls under the rule of not asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or point out a specific issue.
Workflow ID: wflow_qeYt4q7vEMkI1fq8
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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
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.
High Roller Pot lets players gamble gp stakes. Hosts set a stake and payout mode (winner-takes-all or top-3 split) and either invite specific players to confirm within 30 seconds or open the lobby for anyone to join. When the pot starts, all participants (including the host) are charged immediately, each rolls a random tradeable item, and the highest-value roll wins the pot (or top three split the payout).
Important
Add High Roller Pot gambling feature with new command, image rendering, and tests.
high_rollercommand ingamble.ts.drawHighRollerImage()inhighRollerImage.tsto generate result images.highRollerCommand()inhighRollerCommand.tsfor handling game logic.drawHighRollerImage()inhighRollerImage.test.ts.highRollerCommand.test.ts.This description was created by
for c51b267. You can customize this summary. It will automatically update as commits are pushed.