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: various issues preventing impersonated accounts and forks from working #80

Merged
merged 3 commits into from
Jan 5, 2024

Conversation

antazoey
Copy link
Member

@antazoey antazoey commented Jan 4, 2024

What I did

  • fix: issue with static-fee txns where impersonated accounts failed without balance
  • fix: issue where transactions state was lost for proxy-based accounts
  • fix: forked config attr error
  • fix: issue with
  • fix: issue where we would reset the fork to a problematic base fee

fixes: ApeWorX/ape#1783

How I did it

How to verify it

1: set gas price in CLI to default to 0
2: delete hack in code that is no longer necessary (at least I think it is no longer necessary??)

  • So i am referring to the set_code(0) part for impersonated contracts we were doing... we had to do that before but it seems we no longer need to? also it seems to cause problems... (one of many, unfortunately)
    3: set self._fork_config.upstream_provider = upstream_name
    4: delete the part in reset fork where we do self._make_request("anvil_setNextBlockBaseFeePerGas", [base_fee]) as that seems to cause problems and is no longer needed (again, is this actually no longer needed with this PR's changes?? because it just seems to cause problems otherwise that are fixed by deleting that code).

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
  • New test cases have been added and are passing
  • Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

@antazoey
Copy link
Member Author

antazoey commented Jan 4, 2024

NOTE: Tests are failing because they require this PR: ApeWorX/ape#1814

@@ -427,6 +431,8 @@ def build_command(self) -> List[str]:
"--steps-tracing",
"--block-base-fee-per-gas",
f"{self.settings.base_fee}",
"--gas-price",
f"{self.settings.gas_price}",
Copy link
Member

Choose a reason for hiding this comment

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

I believe you can't have both base fee and gas price at the same time

To get a more consistently nice testing environment, I think a gas price of zero works for local testing. For fork testing, I'm not sure if that'll work but it is better than using base fee because that always requires deducting gas

Copy link
Member Author

@antazoey antazoey Jan 5, 2024

Choose a reason for hiding this comment

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

i think it is just 1 setting applies to 1 type of tx and the other applies to another type of tx
also seems like brownie uses pre-1559 txns by default, which is how i got this bug... because i was trying to copy brownie in ape

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it went out of favor around the time EIP-1559 got popular

fubuloubu
fubuloubu previously approved these changes Jan 5, 2024
@antazoey antazoey merged commit c8213d4 into ApeWorX:main Jan 5, 2024
13 checks passed
@antazoey antazoey deleted the fix/fixes branch January 5, 2024 23:22
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.

Contract call execution not completing.
2 participants