feat!: allow transaction raise on fail

[helloibis]

What I did

Begin adding in parameters to facilitate explicitly raising on transaction failure. We'll need transactions that fail to deliberately raise an exception for RevertsContextManager. That functionality is a dependency for the tracing ticket #251.

fixes: #937

How I did it

Adding raise_on_fail: bool parameter to the following:

• ReceiptAPI.await_confirmations() (default False)
• ProviderAPI.get_receipt() (default False)
• ProviderAPI.send_transaction() (default True)
• AccountAPI.call() (default True)
• ImpersonatedAccount.call() (default True)
• Web3Provider.send_transaction() (default True)
• ContractTransaction.__call__() (default True)
• LocalProvider.send_transaction() (default True)

How to verify it

Added a test against flipping this parameter when calling the contract function to ensure that the transaction causes an exception rather than just returning a failed receipt

Checklist

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

[ghost]

cc @NotPeopling2day @fubuloubu @unparalleled-js

Hey all, this is just a draft because I have a few questions:

Test Environment Txn Failures

I think previously we talked about having a transaction raise_on_fail flag's default change based on whether we're in a test suite or not (not just pytest, but ape test for projects). What are everyone's thoughts on that?

Transaction Field / Pydantic Issues

Currently I'm running issues with how to actually define this TransactionAPI.raise_on_fail field I'm adding.

Field(..., exclude=True)

If I go about it the normal Pydantic way:

class TransactionAPI(...):
	...

	raise_on_fail = Field(False, exclude=True)

Then I actually get the field when serializing transactions:

>>> txn = ...  # let's say I have a transaction, like in the updated test
>>> "raise_on_fail" in txn.dict()
True

If I change the definition to Field(None, exclude=True) then I don't run into the issue with serialization but something about that still seems off.

Additionally, there are functions like src/ape/contracts/base.py::_convert_kwargs() which use TransactionAPI.__fields__. Cases like that would need to be updated to exclude this one-off field specifically, and I think I was still having issues after that but would need to try those changes again.

Private field

Prefixing the field with an underscore "hides" it from Pydantic entirely, but then there's the inverse issue of functions like as_transaction() or serialize_transaction() not working with attributes that aren't Pydantic fields.

What are everyone's thoughts? I didn't want to go start changing up serialize_transaction() calls since there seem to be several and I'm not sure if that's really the right approach anyway.

Thanks!

[ghost]

Are we able to avoid adding it to the model if use the same approach that we did for the block_identifier and state_overrides kwarg handling?

@unparalleled-js yeah I think that would be viable and honestly be a little better in "where" the functionality lives (since really this flag doesn't need to be on a transaction, it just needs to be specified when dealing with transactions)

Is there a preferred way to document those kind of kwargs? I see block_identifier coming up in the code but not docstrings or comments

[antazoey]

Is there a preferred way to document those kind of kwargs? I see block_identifier coming up in the code but not docstrings or comments

For yours, we may want to consider adding to 0.5 API changes since it seems common enough across ecosystems to want to do this.

For the other kwargs you mentioned, it got me thinking and this what I landed on so far: https://github.com/ApeWorX/ape/pull/966 and we can do something similar in the meanwhile to avoid having this be a breaking change.

I am open to suggestions though, on that PR.

[ghost]

Are we able to avoid adding it to the model if use the same approach that we did for the block_identifier and state_overrides kwarg handling?

You can find that in ape.api.providers.Web3Provider in methods like estimate_gas_cost()

Yeah, I think so - I gave it a stab in e2c08e01

In the tests test_get_failed_receipt() / test_get_failed_receipt_raise() when I make the call it's using AccountAPI.call(). From there then that calls down to ProviderAPI.send_transaction()

Since those functions only take the txn they'd need additional kwargs added if we didn't append the raise_on_fail field to TransactionAPI. I updated the call signatures to:

AccountAPI.call(self, txn: TransactionAPI, send_everything: bool, raise_on_fail: bool)
ProviderAPI.send_transaction(self, txn: TransactionAPI, raise_on_fail: bool)

Those two aforementioned tests now show the usage:

txn = ...

# non-raising call examples
owner.call(txn, raise_on_fail=False)
provider.send_transaction(txn, raise_on_fail=False)

# raising call examples
owner.call(txn, raise_on_fail=True)
provider.send_transaction(txn, raise_on_fail=True)

cc @unparalleled-js @NotPeopling2day

[ghost]

Rebased off main, some of those commit hashes I just linked might be "old" now haha

[ghost]

With the kwarg being distributed across a few places, is there a preferred method of how we should handle its default value? I think that perhaps it could be a three-way value and we check inside the logic of the different callsites, for example:

def send_transaction(self, ..., raise_on_fail: Optional[bool] = None):
    if raise_on_fail is None and os.environ.get("APE_TESTING", False):
        raise_on_fail = True

cc @unparalleled-js @NotPeopling2day

[fubuloubu]

Not seeing a way for a contract method to pass this parameter

>>> foo.bar(False, sender=dev, raise_on_fail=True)
...

ContractLogicError: Transaction failed.

[ghost]

Not seeing a way for a contract method to pass this parameter

>>> foo.bar(False, sender=dev, raise_on_fail=True)
...

ContractLogicError: Transaction failed.

@fubuloubu that should be covered by https://github.com/ApeWorX/ape/pull/957/commits/a471f77dda2ffa86271f01d262edcfa69b6b9de7 unless I'm missing something

The test examples define setNumber() in the Vyper contract and then can pass raise_on_fail when calling it:

with pytest.raises(TransactionError):
    vyper_contract_instance.setNumber(5, sender=owner, gas_limit=100000, raise_on_fail=True)

Not passing raise_on_fail (or passing False) will return a failed receipt instead

[antazoey]

I tried using this with the ape-hardhat plugin to run tests but I get errors TypeError: send_transaction() got an unexpected keyword argument 'raise_on_fail' so I am 🤔

It did work with local:test and local:geth though.

[ghost]

I tried using this with the ape-hardhat plugin to run tests but I get errors TypeError: send_transaction() got an unexpected keyword argument 'raise_on_fail' so I am 🤔

It did work with local:test and local:geth though.

Yeah I think we'll need corresponding PRs for the different provider plugins since those functions never took **kwargs, working on updating those now

[fubuloubu]

Not seeing a way for a contract method to pass this parameter

>>> foo.bar(False, sender=dev, raise_on_fail=True)
...

ContractLogicError: Transaction failed.

@fubuloubu that should be covered by a471f77 unless I'm missing something

Not passing raise_on_fail (or passing False) will return a failed receipt instead

meant to share when raise_on_fail was False, because it didn't seem to affect it's behavior

[fubuloubu]

overall, I think this might need some good QA over the different circumstances that raise errors

[NotPeopling2day]

@helloibis let's setup a call this afternoon and work through some of it

[ghost]

meant to share when raise_on_fail was False, because it didn't seem to affect it's behavior

@fubuloubu is there a short example of what you mean? In test_get_failed_receipt() I added the raise_on_fail flag and flipping it between True/False causes it to either raise or not raise when the transaction reverts on assert num != 5

[fubuloubu]

will have to do a bit of QA on this

[ghost]

PR is updated and ready for review, I'm getting the corresponding provider plugin PRs ready and adding tests for them

Current State

Just added 6f799560 to set the default value back to True. On our call Friday Bryant and I discussed the behavior of transactions matching the following:

# By default, gas estimation runs (and can raise) and reverted transactions
# raise an exception (even though they're committed to chain)
>>> receipt = contract.foo(...)
TransactionError

# Setting a gas limit can bypass the estimation, but a reverted transaction will still raise
>>> receipt = contract.foo(..., gas_limit=100000)
TransactionError

# Using `raise_on_fail=False` turns off the receipt raising behavior.
# It currently doesn't affect gas estimation but failed receipts do not raise
>>> receipt = contract.foo(..., raise_on_fail=False)
TransactionError    # is gas estimation fails
<ReceiptAPI ...>  # if gas estimation passes

Future State

Later on it should have the following behavior:

# Using `raise_on_fail=False` in the future will also bypass gas estimation. So you can 
>>> receipt = contract.foo(..., raise_on_fail=False)
<ReceiptAPI ...>   # no gas estimation run, always returns receipt

# Inside ape.reverts(), gas estimation always passes but failed receipts still raise
>>> with ape.reverts():
        receipt = contract.foo(...)
TransactionError

Because that implementation is different from the kwargs I'm adding, I think it'll be best suited as a separate PR

[fubuloubu]

Future State

Later on it should have the following behavior:

...

# Inside ape.reverts(), gas estimation always passes but failed receipts still raise
>>> with ape.reverts():
        receipt = contract.foo(...)
TransactionError

Wanted to shout out one difference here, I think in this scenario it should also have the same behavior of bypassing gas estimation (as if raise_on_fail=False was added) and yes also raise up the transaction error, as if this were happening behind the scenes:

receipt = contract.foo(..., raise_on_fail=False)
receipt.raise_for_status()

[antazoey]

I am worried about this (from an ape-console environment):

receipt = contract.foo(..., raise_on_fail=False)

^ this is not working unless I also provide the gas_limit. I am worried this will confuse users. Are we not able to allow the raise_on_fail to work for gas estimation?

[ghost]

I am worried about this (from an ape-console environment):

receipt = contract.foo(..., raise_on_fail=False)

^ this is not working unless I also provide the gas_limit. I am worried this will confuse users. Are we not able to allow the raise_on_fail to work for gas estimation?

@unparalleled-js the issue I'm running into is that if we completely bypass gas estimation, the txn.gas_limit remains None which causes a missing field error later on when signing the transaction (via eth_account.EthAccount, which is requiring the field)

Even if I were to allow gas estimation to run but pass on any exceptions, we still end up with txn.gas_limit is None. Is this something that we can get around or would bypassing gas estimation only work if the user happened to supply their own gas_limit?

Ideally raise_on_fail=False will bypass gas estimation, but I'm blocked on that part by what I just described

cc @NotPeopling2day

[NotPeopling2day]

I am worried about this (from an ape-console environment):

receipt = contract.foo(..., raise_on_fail=False)

^ this is not working unless I also provide the gas_limit. I am worried this will confuse users. Are we not able to allow the raise_on_fail to work for gas estimation?

@unparalleled-js the issue I'm running into is that if we completely bypass gas estimation, the txn.gas_limit remains None which causes a missing field error later on when signing the transaction (via eth_account.EthAccount, which is requiring the field)

Even if I were to allow gas estimation to run but pass on any exceptions, we still end up with txn.gas_limit is None. Is this something that we can get around or would bypassing gas estimation only work if the user happened to supply their own gas_limit?

Ideally raise_on_fail=False will bypass gas estimation, but I'm blocked on that part by what I just described

cc @NotPeopling2day

I might be missing some nuance here, but I think it's that you would supply a gas_limit value during the bypass. I believe Brownie uses an environment configuration to grab a "max" gas based off of the last block in the chain.

https://github.com/eth-brownie/brownie/blob/4ae5f527ea86eb95766fe225a0f67620ffd36022/brownie/network/account.py#L415

[antazoey]

@unparalleled-js the issue I'm running into is that if we completely bypass gas estimation, the txn.gas_limit remains None which causes a missing field error later on when signing the transaction (via eth_account.EthAccount, which is requiring the field)

Thanks for explaining this, that totally makes sense and now I understand the problem! I think that @NotPeopling2day is on to something here- we may need a way for EcosystemAPI to define what its max fee value is. Perhaps a new API property?

[fubuloubu]

@unparalleled-js the issue I'm running into is that if we completely bypass gas estimation, the txn.gas_limit remains None which causes a missing field error later on when signing the transaction (via eth_account.EthAccount, which is requiring the field)

Thanks for explaining this, that totally makes sense and now I understand the problem! I think that @NotPeopling2day is on to something here- we may need a way for EcosystemAPI to define what its max fee value is. Perhaps a new API property?

Max fee limit, or do you mean max gas usage?

[ghost]

Thanks for explaining this, that totally makes sense and now I understand the problem! I think that @NotPeopling2day is on to something here- we may need a way for EcosystemAPI to define what its max fee value is. Perhaps a new API property?

Max fee limit, or do you mean max gas usage?

~Seems like Brownie grabs some configuration values for the max/priority fee if you're not specifying a gas limit for a given txn. Should we have parity with that behavior?~

I also opened up a separate ticket for configuration details at https://github.com/ApeWorX/ape/issues/985. For now I just mentioned gas_limit, but as noted with the Brownie question there may be other, related configuration settings we'd want to add in as well

EDIT: Misread Brownie's code, the max-fee/max-priority stuff is only if you aren't specifying a gas price

[ghost]

@fubuloubu @unparalleled-js PR is ready for review, with the state of the feature still being what's described in https://github.com/ApeWorX/ape/pull/957#issuecomment-1215565267

[ghost]

Just pushed up 59324c0b to fix the default value for raise_on_fail inside get_receipt() / await_confirmations()

If we had left it to True this actually would've been a breaking change, but leaving it to False should still allow it to work as documented earlier (since send_transaction() will raise) without causing any breaking changes

[ghost]

Also just rebased to fix the git conflict

[ghost]

I noticed this breaks ape-hardhat and ape-foundry so we may either have to hold off on merging this to main or do some preparation fixes there first to see if we can mitigate breaking those plugins

Ah yeah, almost forgot about those! I mentioned them earlier but there have been so many comments 😅 will get on those now and make companion PRs that can go in at the same time as this

[ghost]

Moved this to a feature branch since we'll hold til 0.6.0 for the breaking change. This PR should be good to merge now, and here are the accompanying PRs (each with their own feature branch):

https://github.com/ApeWorX/ape-hardhat/pull/99 https://github.com/ApeWorX/ape-foundry/pull/17 https://github.com/ApeWorX/ape-starknet/pull/95 https://github.com/ApeWorX/ape-ganache/pull/8