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/handle all decimal conversions #8

Merged

Conversation

camfairchild
Copy link

This PR fixes an issue with the gas charging on the EVM layer.
The gas was withdrawn at the limit, but the unused gas was not refunded.
This happened because the refund calculation never converted the remainder to the right magnitude, meaning the refund was <1 RAO and refunded as 0 RAO

Copy link

@sam0x17 sam0x17 left a comment

Choose a reason for hiding this comment

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

Looks good to me, some nits:

  • tests covering maximum and minimum values for u64 and U256 in the balance converters would be nice, but the bound checks look correct
  • would be nice if we could get a more detailed description / name for EVM_DECIMALS_FACTOR like EVM_TO_SUBSTRATE_DECIMALS maybe?
  • The into_evm_balance and into_substrate_balance methods use checked_mul and checked_div. We might want to do some logging when the conversion fails

@camfairchild
Copy link
Author

camfairchild commented Jan 7, 2025

Looks good to me, some nits:

* tests covering maximum and minimum values for u64 and U256 in the balance converters would be nice, but the bound checks look correct

* would be nice if we could get a more detailed description / name for `EVM_DECIMALS_FACTOR` like `EVM_TO_SUBSTRATE_DECIMALS` maybe?

* The `into_evm_balance` and into_substrate_balance methods use `checked_mul` and `checked_div`. We might want to do some logging when the conversion fails

See opentensor/subtensor#1129 for most of those fixes
Probably more relevant to include them there

@unconst unconst merged commit 2552620 into opentensor:master Jan 7, 2025
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