feat!: allow transaction raise on fail

[ghost]

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.

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

• All changes are completed
• New test cases have been added
• 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!

[antazoey]

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()

[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:
#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 e2c08e0

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 a471f77 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 6f79956 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]

Just did some testing, this is great! I can get failed receipts easily and my projects' tests still pass (with all their revert-based testing etc).

However -- dun dun dun. 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

[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

[fubuloubu]

looking good

[antazoey]

LGTM

[antazoey]

This part of the test might make more sense in test_contract_instance.py

def test_invoke_failing_method_without_raising()

[ghost]

Can do, I'll take a look at cleaning up some test stuff in the upcoming PR for reverts since that will need to add in testing changes

[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):

ApeWorX/ape-hardhat#99
ApeWorX/ape-foundry#17
ApeWorX/archive-ape-starknet#95
ApeWorX/ape-ganache#8