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

Fix #702: Check balances before submitting a prediction #713

Conversation

trizin
Copy link
Contributor

@trizin trizin commented Mar 1, 2024

Fixes #702

  • Abort prediction submission if one of the wallets has insufficient balance

@trizin trizin linked an issue Mar 1, 2024 that may be closed by this pull request
@trizin trizin changed the title Add and use check_balances function #702 - Check balances before submitting a prediction Mar 1, 2024
@trentmc trentmc changed the title #702 - Check balances before submitting a prediction Fix #702: Check balances before submitting a prediction Mar 3, 2024
@trizin
Copy link
Contributor Author

trizin commented Mar 5, 2024

Gonna be ready for review once #733 is merged

@trizin trizin marked this pull request as ready for review March 7, 2024 20:38
@trizin trizin requested a review from trentmc March 7, 2024 20:38
Comment on lines 356 to 357
up_predictoor_address = self.web3_config_up.owner
down_predictoor_address = self.web3_config_down.owner
Copy link
Member

Choose a reason for hiding this comment

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

Make this simpler: have new properties to conveniently get the addresses. And keep the names shorter. That is:

@property
def up_addr(self) -> str:
        return self.web3_config_up.owner

@property
def down_addr(self) -> str:
        return self.web3_config_down.owner

Which means you can delete

        up_predictoor_address = self.web3_config_up.owner
        down_predictoor_address = self.web3_config_down.owner

and instead make calls to the properties instead.

Comment on lines 359 to 360
minimum_ocean_balance = self.ppss.predictoor_ss.stake_amount.to_wei()
minimum_native_balance = Eth(1).to_wei()
Copy link
Member

Choose a reason for hiding this comment

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

Let's aim to make variable names shorter, yet just as informative. Some guidelines...

Whenever we use token amounts, use capital letters.

And for "native" tokens we can be specific to Sapphire, since that's all Predictoor will be on for the foreseeable future.

Therefore "ocean" -> "OCEAN", and "native" -> "ROSE".

"balance" can be "bal"; "minimum" can be "min".

"bal" should be after "OCEAN" or "ROSE".

We don't need to say "predictoor" because "up" and "down" will implicitly tell enough.

Therefore we can relabel:

  • "up_predictoor_balance_ocean" -> "up_OCEAN_bal"
  • "down_predictoor_balance_ocean" -> "down_OCEAN_bal"
  • "minimum_ocean_balance" -> "min_OCEAN_bal"
  • "minimum_native_balance" -> "min_ROSE_bal"

Second, please use the new currency types "Eth" and "Wei". Will minimize chance of error.

Do this throughout all your changes please.


With the two suggestions above, the two lines above look like:

        min_OCEAN_bal = self.ppss.predictoor_ss.stake_amount # in units of Eth
        min_ROSE_bal = Eth(1) # in units of Eth

Copy link
Contributor Author

@trizin trizin Mar 8, 2024

Choose a reason for hiding this comment

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

All done 🙌 thanks!

I couldn't find the part where new currency types "Eth" and "Wei" weren't used. Could you please point out that part so I can fix it?

Please note that balanceOf returns Wei.

Comment on lines 366 to 369
logger.error(
"Up predictoor's OCEAN balance too low: (%s)",
up_predictoor_balance_ocean,
)
Copy link
Member

Choose a reason for hiding this comment

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

Change all log messages such that they take up 1 line.

Copy link
Contributor Author

@trizin trizin Mar 8, 2024

Choose a reason for hiding this comment

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

I'm not sure if it's possible to shrink this message to a single line.

edit: Removed "predictoor's" from the message, that did it


@enforce_types
@pytest.mark.parametrize(
"up_ocean, down_ocean, up_native, down_native, expected",
Copy link
Member

Choose a reason for hiding this comment

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

Rename "up_ocean, down_ocean, up_native, down_native, expected"

to

"up_OCEAN, down_OCENA, up_ROSE, down_ROSE, expected"

(in line with my previous comment)

Comment on lines +363 to +364
def check_balances(self) -> bool:
min_OCEAN_bal = self.ppss.predictoor_ss.stake_amount.to_wei()
Copy link
Member

Choose a reason for hiding this comment

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

As discussed in standup, add two new properties: OCEAN() and ROSE(). From that, you'll be able to simplify check_balances() further. Including: you won't need local variables for balances or tokens.

Copy link
Member

@trentmc trentmc left a comment

Choose a reason for hiding this comment

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

See new comment

@trizin trizin requested a review from trentmc March 12, 2024 13:31
@trizin trizin merged commit 14aaa88 into main Mar 18, 2024
5 checks passed
@trizin trizin deleted the issue702-pdr-bot-check-balances-of-both-wallets-before-submitting-prediction branch March 18, 2024 15:27
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.

[Pdr bot] Check balances of both wallets before submitting prediction
2 participants