Skip to content

changed wages and added transfer - #8

Open
vmbbi wants to merge 3 commits into
masterfrom
wages-and-transfer
Open

vmbbi wants to merge 3 commits into
masterfrom
wages-and-transfer

Conversation

@vmbbi

@vmbbi vmbbi commented Sep 17, 2026

Copy link
Copy Markdown
Member

added ability to transfer money between users and made it so config can define how long till you can collect next wage

@vmbbi
vmbbi requested review from drtheodor and rapbattlegod32 and a balanced review from Copilot September 17, 2026 16:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The transfer transaction currently fails for senders without an existing profile row and the wage/transfer config + formatting includes correctness issues that will surface at runtime.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/commands/fun/economy.ts:998

  • This check no longer rejects negative balances, but config.economy.setBalance.invalid says the valid range is 0..2B. Allowing negative balances can also break assumptions elsewhere (e.g., affordability checks).
    if (Math.abs(amount) > 2_000_000_000) return void await interaction.editReply({ content: config.economy.setBalance.invalid });
  • Files reviewed: 2/2 changed files
  • Comments generated: 7
  • Review effort level: Lite

Comment thread config.json.js Outdated
Comment thread src/commands/fun/economy.ts
const amount = interaction.options.getInteger("amount", true);

if (amount <= 0 || amount > 1000000000) return void await interaction.editReply({ content: config.economy.limit });
if (amount === 0 || Math.abs(amount) > 1000000000) return void await interaction.editReply({ content: config.economy.limit });
Comment thread src/commands/fun/economy.ts
Comment thread src/commands/fun/economy.ts Outdated
Comment thread src/commands/fun/economy.ts Outdated
});
}
console.error("[Economy Pay] Transaction Error:", error);
return void await interaction.editReply({ content: "wire transfer failed. Try again later" });

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new wage-claim persistence/concurrency behavior has correctness issues (missing uniqueness for upsert and non-atomic eligibility checks) that can lead to incorrect payouts/cooldowns.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

src/commands/fun/economy.ts:1083

  • /economy set-balance is described as setting an exact balance and the configured error message says the valid range is 0..2B, but this validation now allows negative balances. If negative balances are not intended, restore the non-negative check (or update the command/config text to match the new behavior).

src/commands/fun/economy.ts:1494

  • This error response is hard-coded, lowercased, and bypasses the configurable economy messages you added for transfers. Consider using a config-driven message (e.g. config.economy.transfer.database_error) so server owners can localize/tune it and it stays consistent with other economy errors.
        return void await interaction.editReply({ content: "wire transfer failed. Try again later" });

src/commands/fun/economy.ts:1062

  • /economy add-money is described as adding money and config.economy.limit says the amount must be bigger than 0, but the current validation allows negative amounts. Also, after profile.increment(...), using profile.balance + amount can produce an incorrect newBalance (depending on whether the instance was updated/reloaded). Enforce a positive amount and fetch the updated balance for the response.
    if (amount === 0 || Math.abs(amount) > 1000000000) return void await interaction.editReply({ content: config.economy.limit });
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread src/commands/fun/economy.ts Outdated
Comment thread src/commands/fun/economy.ts Outdated

@drtheodor drtheodor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what he said

@vmbbi
vmbbi requested review from drtheodor and a lite review from Copilot September 22, 2026 12:17

Copilot AI left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

poor.

@vmbbi
vmbbi requested a lite review from Copilot September 22, 2026 12:19

Copilot AI left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

poor.

@vmbbi
vmbbi requested a lite review from Copilot September 24, 2026 00:43

Copilot AI left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still poor.

@vmbbi
vmbbi requested a lite review from Copilot September 30, 2026 12:42

Copilot AI left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Poor

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unresolved validation, transfer visibility/messaging, and wage-role selection issues block approval.

Review effort: Lite
Findings: 1 Medium severity · 1 Low severity

Open (2)
Resolved since last review (2)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Consistently select the highest-paying wage role

src/​commands/​fun/​economy.ts:520

The newly added getMemberWageConfig explicitly selects the highest-paying matching role, but handleWage never calls it and instead adds every matching role to qualifiedWages. A member with multiple configured roles will therefore receive and independently cooldown multiple salaries, changing the previous Math.max behavior and making the two implementations inconsistent. Use one selection strategy consistently (or remove the unused helper if stacking is intentional).

Medium severity Prevent set-balance from allowing negative balances

src/​commands/​fun/​economy.ts:1098

This changes set-balance to permit negative balances, contradicting the configured error message (Invalid amount range (0 to 2B).) and the previous non-negative contract. That also leaves users unable to use normal positive-only spending commands until another administrative correction; reject values below zero unless negative balances are an intentional, documented feature.

Low severity Use transfer-specific insufficient-funds messaging

src/​commands/​fun/​economy.ts:1467

The insufficient-funds response uses the generic cantAfford text, which ends with “You don't have enough money to bet!” even though this is a transfer. This gives the recipient an incorrect explanation; add a transfer-specific insufficient-funds message and use it for this check (and the transaction-failure branch below).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants