fix: logging with multiple(external) logs

[hal3e]

Fixes two bugs we had with decoding logs from RevertTransactionError:

1. Take the last log instead of the first log.
   If we had several log statements before the require we would take the first one and that would be wrong. The last log should be the log from the require statement.

2. If we had a script calling a contract which was logging some variables our decode would fail. This was because we could not find the logId in our LogDecoder. Now we just do a filter_map and take only the logs we have in our LogDecoder.

Added test:

• logs before require
• e2e test where script is calling a contract that makes some logs

[segfault-magnet]

Can you confirm whether I understood this correctly:

If we had a script calling a contract which was logging some variables our decode would fail. This was because we could not find the logId in our LogDecoder.

Is this the case where we load a (foreign-non-SDK-generated) script binary and execute it but it turns out the script bytecode had called a contract that logged something but we can't decode it since we don't have the contract's abi-json?

If so, do we have a test covering this scenario?

[hal3e]

Can you confirm whether I understood this correctly:

If we had a script calling a contract which was logging some variables our decode would fail. This was because we could not find the logId in our LogDecoder.

Is this the case where we load a (foreign-non-SDK-generated) script binary and execute it but it turns out the script bytecode had called a contract that logged something but we can't decode it since we don't have the contract's abi-json?

If so, do we have a test covering this scenario?

Yes that was the case. We would have a (contract_id, log_id) entry that was not in our LogDecoder. For more clarity the problem was not that it was not there but how we handled that case. Previously, we would return an error and that would mean that we can not decode a log and that we return the initial revert error message. Now, we just filter them out. We can do this because the require will generate a log and there will be a valid log_id in the receipts. IMO we do not need a test that shows that it filters out non existing entries.. It is obvious from the code.

That said, it would be cool if we could use your shared types PR, where we can simultaneously load several contracts and scripts, to combine all LogDecoders into one and pass it down. This would mean that we can do complex logging.

[segfault-magnet]

Can you confirm whether I understood this correctly:

If we had a script calling a contract which was logging some variables our decode would fail. This was because we could not find the logId in our LogDecoder.

Is this the case where we load a (foreign-non-SDK-generated) script binary and execute it but it turns out the script bytecode had called a contract that logged something but we can't decode it since we don't have the contract's abi-json?
If so, do we have a test covering this scenario?

Yes that was the case. We would have a (contract_id, log_id) entry that was not in our LogDecoder. For more clarity the problem was not that it was not there but how we handled that case. Previously, we would return an error and that would mean that we can not decode a log and that we return the initial revert error message. Now, we just filter them out. We can do this because the require will generate a log and there will be a valid log_id in the receipts.

IMO we do not need a test that shows that it filters out non existing entries.. It is obvious from the code.

That said, it would be cool if we could use your shared types PR, where we can simultaneously load several contracts and scripts, to combine all LogDecoders into one and pass it down. This would mean that we can do complex logging.

Tnx for the clarification.

We filter out/ignore log receipts for which we do not know the type. And the only way that could happen is by running foreign contract calling scripts.

We can do this because the require will generate a log and there will be a valid log_id in the receipts

I'm not sure what you wanted to say. We must filter out precisely because we don't have the mapping for those log_ids. Presumably, you're saying that the way log decoding works is by having the log_id->type mapping provided in the json-abi. I'm ok with that, it's just that the filtering is introduced for those cases where we don't have the json-abi for the contract that logged the log.

IMO we do not need a test that shows that it filters out non-existing entries. It is obvious from the code.

It has naught to do with clarity. We have behavior where we're willing to ignore invalid/stray log receipts but we're (possibly) not testing it. So this fix will not have any protection against regression.

[hal3e]

Makes sense. I will add a unit test for this.

[hal3e]

At the end I decided to add a e2e test as we do not have any test where a script is calling a contract.

[digorithm]

Very nice!