Skip to content
This repository has been archived by the owner on Jan 8, 2025. It is now read-only.

Implement eth_get_balance #990

Merged
merged 4 commits into from
Sep 30, 2024
Merged

Conversation

saimeunt
Copy link
Contributor

@saimeunt saimeunt commented Sep 28, 2024

This PR implements eth_get_balance and use it instead of querying the balanceOf function of the native_token in the codebase. It also fixes a suspected bug in get_starknet_address from KakarotCore.

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

The eth_get_balance is not implemented and thus we need to query the native_token directly in several parts of the codebase.

Resolves: #941

What is the new behavior?

The eth_get_balance is now implemented and replicates its eth_getBalance counterpart in the EVM.

  • The implementation of eth_get_balance leverages the get_starknet_address function from KakarotCore to get the corresponding starknet address from an evm address. Once we have the underlying starknet address, we query the native_token balance. The function has also been added to ExtendedKakarotCore interface.
  • When reading about the description of the get_starknet_address, I suspected a bug and from my understanding we should return the registered starknet address only if it is found and thus non-zero, so I think the if condition should be inversed, this is why my PR introduces a bugfix.
  • The occurences of direct native_token querying have been replaced by eth_get_balance, in particular for the eth_validate_tx test helper I had to mock additional calls so account.get_evm_address returns the expected account_starknet_address.
  • A simple test has been added to ensure the correct behavior of eth_get_balance both for uninitialized and initialized accounts.

Does this introduce a breaking change?

  • Yes
  • No

This change is Reviewable

Copy link
Contributor

@enitrat enitrat 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!

crates/evm/src/backend/validation.cairo Outdated Show resolved Hide resolved
crates/evm/src/backend/validation.cairo Outdated Show resolved Hide resolved
crates/evm/src/backend/validation.cairo Outdated Show resolved Hide resolved
@saimeunt
Copy link
Contributor Author

saimeunt commented Sep 28, 2024

@enitrat Thank you for your quick review and your Cairo tips, I made the requested changes by following your best practices insights.

I will apply to #991 as I'd like to continue my work on Kakarot so this is appreciated! 🙌

enitrat
enitrat previously approved these changes Sep 29, 2024
@enitrat enitrat merged commit 5759f30 into kkrt-labs:main Sep 30, 2024
2 of 3 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: implement eth_get_balance
2 participants