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

feat: add set_balance test provider API #823

Merged
merged 11 commits into from
Jul 1, 2022

Conversation

antazoey
Copy link
Member

What I did

Adds set_balance() API to TestProviderAPI class that raises not implemented by default.
Hardhat and Foundry both use this!

How I did it

How to verify it

Checklist

  • All changes are completed
  • New test cases have been added
  • Documentation has been updated

fubuloubu
fubuloubu previously approved these changes Jun 23, 2022
@antazoey
Copy link
Member Author

We can better out-of-the-box support if this feat request is honored:

ethereum/eth-tester#236

Also, I will adjust + add more tests to ape-hardhat and ape-foundry where the API is actually implemented.

@antazoey antazoey requested a review from NotPeopling2day June 30, 2022 16:26
fubuloubu
fubuloubu previously approved these changes Jul 1, 2022
Comment on lines +764 to +765
if isinstance(account, BaseAddress):
account = account.address # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't self.conversion_manager.convert(account, AddressType) work here?

Comment on lines +771 to +772
# Support hex strings
amount = int(amount, 16)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't there a hex converter toint?



def test_set_balance(chain, test_accounts):
with pytest.raises(APINotImplementedError):
Copy link
Member

Choose a reason for hiding this comment

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

Wait, does eth-tester not support this method?

Copy link
Member Author

Choose a reason for hiding this comment

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

It didnt seem like it but i created an issue for them

Comment on lines +382 to +383
@raises_not_implemented
def set_balance(self, address: AddressType, amount: int):
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for discussing this choice and future alternatives to making these test methods, like setting the balance or timestamp, only available on local/test chains.

Copy link
Contributor

@NotPeopling2day NotPeopling2day left a comment

Choose a reason for hiding this comment

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

LGTM

@NotPeopling2day NotPeopling2day enabled auto-merge (squash) July 1, 2022 19:35
@NotPeopling2day NotPeopling2day merged commit bdd1fb7 into ApeWorX:main Jul 1, 2022
@antazoey antazoey deleted the feat/jules/set-bal branch July 1, 2022 20:03
PatrickAlphaC pushed a commit to PatrickAlphaC/ape that referenced this pull request Jul 6, 2022
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