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

Add a currency along with various uses #296

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

flaree
Copy link
Member

@flaree flaree commented Jun 27, 2024

Adds a currency (configurable name in config).

TODO:

  • Add configurable currency and name.
  • Add rarity tiers and assignable value and coin amount.
  • Gain X coins on each countryball catch, depending on ball rarity.
  • Add balls dispose - dispose of a ball for 1/2 the worth.
  • Add player balance to view your balance.
  • Add ability to use coins in trades.
  • Wandering 'salesman' who will sell random countryballs, will be a random spawn similar to countryballs but rarer. (possibly out of scope)

Future work for later additions:

  • Marketplace with buying, selling and auctions
  • Achievements will give coins depending on their 'hardness'

@flaree flaree requested a review from laggron42 June 27, 2024 14:19
@Kowlin Kowlin self-requested a review August 21, 2024 13:08
@Kowlin
Copy link
Contributor

Kowlin commented Aug 21, 2024

Mention me when this is ready for review. Wanna have a second pair of eyes on it.

Copy link
Contributor

@Kowlin Kowlin left a comment

Choose a reason for hiding this comment

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

The issue from packages/trade/menu#L318 has to be fixed before this can be merged.

if self.coins < amount:
raise ValueError("Not enough coins")
self.coins -= amount
await self.save(update_fields=("coins",))
Copy link
Contributor

Choose a reason for hiding this comment

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

add_coins and remove_coins should probably return some output on its status as confirmation.

Copy link
Member

Choose a reason for hiding this comment

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

I think raising an error is appropriate as a way to confirm things went well

@@ -621,6 +621,44 @@ async def count(
f"{country}{settings.collectible_name}{plural}{guild}."
)

@app_commands.command()
async def dispose(self, interaction: discord.Interaction, ball: BallInstanceTransform):
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if dispose is the best naming for this command, it seems a bit negative. perhaps exchange

Copy link
Member

Choose a reason for hiding this comment

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

dispose sounds fine to me, exchange may be confused with trading. What about sell maybe?

ballsdex/packages/trade/cog.py Outdated Show resolved Hide resolved
ballsdex/packages/trade/menu.py Outdated Show resolved Hide resolved
@MaxxMXSZ MaxxMXSZ added package:countryballs Package handling countryball spawning, no commands there package:players Package with /player commands package:trade All trade commands package:admin All /admin commands and its configuration options package:balls Package containing /balls commands priority:normal new feature labels Oct 12, 2024
@MaxxMXSZ MaxxMXSZ requested a review from Kowlin October 16, 2024 15:25
@MaxxMXSZ MaxxMXSZ marked this pull request as ready for review October 16, 2024 15:26
@MaxxMXSZ
Copy link
Contributor

About the wandering trader it would be better to make a separate pr for it after coin have been merge. Mainly to evaluate how much members value the coin to give the set value for shops!

Copy link
Contributor

@imtherealF1 imtherealF1 left a comment

Choose a reason for hiding this comment

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

this pr also breaks the trade display and that needs some fixes

ballsdex/packages/balls/cog.py Outdated Show resolved Hide resolved
ballsdex/packages/balls/cog.py Outdated Show resolved Hide resolved
ballsdex/packages/trade/cog.py Outdated Show resolved Hide resolved
ballsdex/packages/trade/cog.py Outdated Show resolved Hide resolved
ballsdex/settings.py Outdated Show resolved Hide resolved
ballsdex/settings.py Show resolved Hide resolved
@MaxxMXSZ
Copy link
Contributor

Putting this as DRAFT again, i will run through all the changes myself and fix some broken stuff.

@MaxxMXSZ MaxxMXSZ marked this pull request as draft October 27, 2024 14:54
@MaxxMXSZ MaxxMXSZ marked this pull request as ready for review October 28, 2024 12:25
@OLi51
Copy link

OLi51 commented Dec 12, 2024

this is my idea :(

@OLi51
Copy link

OLi51 commented Dec 12, 2024

except i used different command names

ballsdex/packages/admin/cog.py Outdated Show resolved Hide resolved
ballsdex/packages/trade/cog.py Outdated Show resolved Hide resolved
ballsdex/packages/trade/menu.py Outdated Show resolved Hide resolved
@imtherealF1
Copy link
Contributor

also, if config.yml isn't updated hence no rarities, /dispose will give a KeyError: 'legendary', and in general settings.rarities = content.get("rarities", {}) doesnt work

@laggron42
Copy link
Member

this is my idea :(

@OLi51 This was considered since before Ballsdex was even released to the public. We haven't looked for any fork or other code for getting inspirations (unless they bring it up to us as suggestions), and besides it's not hard to get that kind of idea.

I don't really understand the goal of your comment? You don't have any code uploaded, there's no contribution from you. If you want to add features, you are free and welcome to open pull requests, then you'll be properly credited. I find it kinda sad to see people developing private features for themselves and wanting exclusivity, when I want to encourage sharing and contributions, that's why the bot is open source to begin with.

@flaree flaree requested a review from Copilot December 13, 2024 15:44
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 13 changed files in this pull request and generated 1 comment.

Files not reviewed (8)
  • migrations/models/36_20241015161531_update.sql: Language not supported
  • ballsdex/packages/trade/display.py: Evaluated as low risk
  • ballsdex/packages/countryballs/components.py: Evaluated as low risk
  • ballsdex/core/utils/enums.py: Evaluated as low risk
  • ballsdex/packages/trade/trade_user.py: Evaluated as low risk
  • ballsdex/settings.py: Evaluated as low risk
  • ballsdex/core/admin/resources.py: Evaluated as low risk
  • ballsdex/core/models.py: Evaluated as low risk
Comments suppressed due to low confidence (3)

ballsdex/packages/trade/menu.py:203

  • The comment should be 'updates every 5 seconds'.
A loop task that updates each 5 second the menu with the new content.

ballsdex/packages/trade/menu.py:308

  • The error message for InvalidTradeOperation should be more descriptive.
raise InvalidTradeOperation()

ballsdex/packages/trade/menu.py:333

  • The comment should be 'This is an invalid mutation'.
// This is a invalid mutation, the player is not the owner of the countryball

ballsdex/packages/trade/menu.py Show resolved Hide resolved
if self.coins < amount:
raise ValueError("Not enough coins")
self.coins -= amount
await self.save(update_fields=("coins",))
Copy link
Member

Choose a reason for hiding this comment

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

I think raising an error is appropriate as a way to confirm things went well

Comment on lines +1562 to +1564
user: discord.User | None = None,
user_id: str | None = None,
amount: int = 0,
Copy link
Member

Choose a reason for hiding this comment

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

amount should be a required parameter

Suggested change
user: discord.User | None = None,
user_id: str | None = None,
amount: int = 0,
amount: int,
user: discord.User | None = None,
user_id: str | None = None,

Comment on lines +1623 to +1625
user: discord.User | None = None,
user_id: str | None = None,
amount: int = 0,
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Suggested change
user: discord.User | None = None,
user_id: str | None = None,
amount: int = 0,
amount: int,
user: discord.User | None = None,
user_id: str | None = None,

@@ -621,6 +621,44 @@ async def count(
f"{country}{settings.collectible_name}{plural}{guild}."
)

@app_commands.command()
async def dispose(self, interaction: discord.Interaction, ball: BallInstanceTransform):
Copy link
Member

Choose a reason for hiding this comment

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

dispose sounds fine to me, exchange may be confused with trading. What about sell maybe?

self.coins -= amount

async def fetch_player_coins(self) -> int:
player, _ = await Player.get_or_create(discord_id=self.user.id)
Copy link
Member

Choose a reason for hiding this comment

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

Makes the query a little lighter. Could also reuse the existing Player model and call .refresh_from_db

Suggested change
player, _ = await Player.get_or_create(discord_id=self.user.id)
player, _ = await Player.get_or_create(discord_id=self.user.id).only("coins")

Comment on lines +176 to +177
self.initial_coins_trader1 = trader1.coins
self.initial_coins_trader2 = trader2.coins
Copy link
Member

Choose a reason for hiding this comment

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

These attributes are unused

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature package:admin All /admin commands and its configuration options package:balls Package containing /balls commands package:countryballs Package handling countryball spawning, no commands there package:players Package with /player commands package:trade All trade commands priority:normal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants