Skip to content
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

Issue# 307 Add Receiver Confirmation to Coin Transfer #456

Merged
merged 11 commits into from
Mar 6, 2023
Merged

Conversation

Fan-Yang-284
Copy link
Contributor

@Fan-Yang-284 Fan-Yang-284 commented Mar 5, 2023

Summary of Changes

Add confirmation to existing transfer feature via interaction handler.

Related Issues

Resolves Issue#-307

Demonstration of Changes

image
image
image
image

Further Information and Comments

Implementation is meant to be mimicked from that of rps.
Added transfer interaction handler as well as Transfer and TransferTracker classes to components/coin.ts
Added updateMessageEmbed to utils/embed.ts, changed rps interaction handler to use updateMessageEmbed
Added UserCoinEvent.CoinTransferSender, UserCoinEvent.CoinTransferReceiver enum values

@Fan-Yang-284 Fan-Yang-284 added the request Request for a new feature or other modification label Mar 5, 2023
@Fan-Yang-284 Fan-Yang-284 changed the title Issue# 307 Issue# 307 Add Receiver Confirmation to Coin Transfer Mar 5, 2023
Copy link
Collaborator

@mcpenguin mcpenguin left a comment

Choose a reason for hiding this comment

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

Great work - just some comments.

new MessageButton()
.setCustomId(`transfer-check-${this.transferId}`)
.setLabel(getEmojiFromSign(TransferSign.Accept))
.setStyle('SECONDARY'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for this we should set the success style to be SUCCESS And the failure style to be FAILURE. Then we can just change the button contents to be "Accept" and "Reject" respectively.

await adjustCoinBalanceByUserId(
this.state.receiver.id,
this.state.amount,
UserCoinEvent.AdminCoinAdjust,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be AdminCoinAdjust. Maybe make a new enum value UserCoinEvent.CoinTransferAcceptor for this?

Comment on lines 76 to 79
if (transfer.transferMessage instanceof Message) {
transfer.transferMessage.edit(<MessagePayload>message);
} else if (transfer.transferMessage instanceof CommandInteraction) {
transfer.transferMessage.editReply(<MessagePayload>message);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think extract this functionality into a helper function in utils for editing a SapphireSentMessageType and use that here.

await adjustCoinBalanceByUserId(
this.state.sender.id,
<number>(-1 * this.state.amount),
UserCoinEvent.AdminCoinAdjust,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be AdminCoinAdjust. Maybe make a new enum value UserCoinEvent.CoinTransferReceiver for this?

return this.transfers.get(id);
}

runFuncOnGame(transferId: string, func: (transfer: Transfer) => void): void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change this method call into something else, since I think a Game does not make sense in this context. Maybe runFuncOnTransfer?

Copy link
Collaborator

@mcpenguin mcpenguin left a comment

Choose a reason for hiding this comment

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

One small comment.

@@ -380,7 +381,7 @@ export class Transfer {
await adjustCoinBalanceByUserId(
this.state.receiver.id,
this.state.amount,
UserCoinEvent.AdminCoinAdjust,
UserCoinEvent.CoinTransferAcceptor,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be CoinTransferReceiver?

Copy link
Collaborator

@mcpenguin mcpenguin left a comment

Choose a reason for hiding this comment

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

LGTM!

@Fan-Yang-284 Fan-Yang-284 merged commit 92edc44 into main Mar 6, 2023
@Fan-Yang-284 Fan-Yang-284 deleted the issue#-307 branch March 6, 2023 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request Request for a new feature or other modification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make coin transfer subcommand
2 participants