-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
GH-43 Add limit on the amount of money sent in one transaction. #47
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe recent changes involve several updates across different classes related to the economy plugin. In the The Furthermore, the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Outside diff range and nitpick comments (3)
eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/PluginConfig.java (1)
23-25
: Nice addition! A few thoughts to consider.The new limit looks good, but maybe we should think about making it easier to change? Also, is a billion the right starting point? It might be a bit high for some setups.
What do you think about these ideas?
- Make
transactionLimit
configurable outside the code.- Start with a lower default, like 100,000.
- Add a quick note in the code about why we picked the default value.
eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessagesPlayerSubSection.java (1)
30-31
: Good addition!The new message looks great and fits well with the others. Nice job!
Maybe consider adding a space after "➤" for consistency with other messages?
eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyTransferCommand.java (1)
25-25
: Consider making 'pluginConfig' final.Since 'pluginConfig' is assigned once and not modified, marking it as
final
can improve code safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java (1 hunks)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyTransferCommand.java (3 hunks)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/PluginConfig.java (1 hunks)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessagesPlayerSubSection.java (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java (1)
122-122
: Looks good! Just one thing to check.The change makes sense for adding a transaction limit. Make sure the
MoneyTransferCommand
class uses this new config properly.Let's double-check how the limit is used:
✅ Verification successful
Let's gather more details on how
pluginConfig
is used inMoneyTransferCommand
.
All set! The transaction limit is properly enforced in
MoneyTransferCommand
.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for the limit implementation in MoneyTransferCommand rg --type java "class MoneyTransferCommand" -A 20Length of output: 2905
Script:
#!/bin/bash # Search for all usages of pluginConfig in MoneyTransferCommand.java rg --type java "pluginConfig" eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyTransferCommand.javaLength of output: 420
eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyTransferCommand.java (4)
7-7
: Good addition of PluginConfig import.This import allows the class to access configuration settings.
30-31
: Constructor updated appropriately.Including
PluginConfig
in the constructor ensures it's properly injected.
36-36
: Proper assignment of pluginConfig.Assigning
pluginConfig
to the class field is done correctly.
52-60
: Transaction limit check looks good.The code correctly checks the amount against the transaction limit and notifies the user if it exceeds.
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.
Overall good job
@@ -27,4 +27,6 @@ public class MessagesPlayerSubSection extends OkaeriConfig { | |||
public Notice transferReceived = Notice.chat("<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> " | |||
+ "<dark_gray>➤</dark_gray> <white>Received <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient> from " | |||
+ "<gradient:#00FFA2:#34AE00>{PLAYER}</gradient>.</white>"); | |||
public Notice transferLimit = Notice.chat("<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> <dark_gray>➤</dark_gray>" | |||
+ " <white>Transaction limit is <gradient:#00FFA2:#34AE00>{LIMIT}</gradient>.</white>"); |
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.
Maybe make it red so the user knows it is above limit and transaction is not successful
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.
it won't match the plugin's color scheme, let's leave it as it is
...leconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyTransferCommand.java
Outdated
Show resolved
Hide resolved
...leconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyTransferCommand.java
Outdated
Show resolved
Hide resolved
…and/player/MoneyTransferCommand.java
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
🧹 Outside diff range and nitpick comments (1)
eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyTransferCommand.java (1)
52-60
: Nice work on the limit check!The transaction limit is well implemented. A small tweak could make it clearer:
-if (amount.compareTo(this.pluginConfig.transactionLimit) > 0) { +if (amount.compareTo(this.pluginConfig.transactionLimit) > 0) {This change makes the comparison more explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyTransferCommand.java (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyTransferCommand.java (3)
7-7
: Looks good!The new import for
PluginConfig
is correctly added.
25-25
: Good job!The new
PluginConfig
field is added and properly initialized in the constructor.Also applies to: 30-31, 36-36
Line range hint
1-85
: Great job implementing the transaction limit!The changes nicely add the limit check to the money transfer process. It fits well with the existing code and should help control transaction sizes.
No description provided.