Include address in logs

[birchmd]

The logs returned in SubmitResult omit the address which created the log, but it seems like an important piece of information when trying to understand what happened in a complex transaction. Etherscan also uses this to search for events, e.g. all USDT transfers.

This issue is to include the address field. This is a breaking change because it changes the SubmitResult binary format.

[birchmd]

I think this might be useful for #245 because otherwise a malicious contract could spoof the event to trick the front-end into thinking it was an exit precompile call. @paouvrard can you confirm that having the address associated with the log entry would be useful for you?

[artob]

@birchmd Can you give an example of when/how the originating address here would be different than the originating address for the transaction that emitted the log? (That's how we index logs right now.)

[birchmd]

@artob The address of the log will always be different than the originating address of the transaction. The address of the log will be that of the smart contract which created the log entry, while the address for the transaction will be an externally owned account (not a smart contract).

Associating log entries with smart contract addresses is useful because it makes clear the steps that occurred in a complex transaction. For example, ERC-20 contracts based on the Open Zeppelin implementation all have a Transfer event. Now, suppose a uniswap transaction involves exchanging multiple ERC-20 tokens. If each Transfer entry in the logs has the smart contract address associated with it then we can trace what tokens were exchanged during this transaction.

Additionally, from a compatibility perspective this is required. The Solidity docs are quite clear that "these logs are associated with the address of the contract".

[artob]

@birchmd Thanks. Please open, going forward, with such justification and rationale when suggesting breaking the ABI.

Associating log entries with smart contract addresses is useful [...] Additionally, from a compatibility perspective this is required

It's not a matter of useful or seemingly useful, it's a matter of correct and compatible. So, clearly this does need fixing.

Now, given that we're breaking this ABI yet again, might I suggest that we also add a version byte so that we have some chance of cleanly dealing with all this going forward? Recall that we need to index (and in the future re-index) all EVM transactions since the launch of the EVM, using the same code base; the previous try-catch logic for dealing with the previous ABI change will need to be thrown out given this new change, now.

Looping in @0x3bfc and @antonpaisov as this will affect the Relayer/Indexer code base the most.

[birchmd]

I suggest that we also add a version byte

We are using borsh serialization for the binary format, so we cannot arbitrarily add a version byte. We could add a version field to the struct (and put it first so the version is always first), or we could represent SubmitResult as an enum (then the byte for the enum variant becomes the version byte), or we could deviate from borsh to include the version byte (binary format becomes VERSION + borsh_serialization). Which of these options do you think is best?

the previous try-catch logic for dealing with the previous ABI change will need to be thrown out given this new change

Why thrown out? We could extend it to include a case for this new format as well.

[artob]

Which of these options do you think is best?

It should remain Borsh-enveloped. So, I was thinking a version field as the first struct element, but the enum should be equivalent on the wire. (In case it isn't, then may need to prefer the version field as Borsh libraries for non-Rust languages are not superb and advanced Borsh constructs become unwieldy or unparseable.)

[joshuajbouw]

Version field to the struct is perfectly fine. Let's ensure that everything is correct with testing from aurora.js first before tackling this. We have to ensure with utmost certainty that we are confident with tests to back it up that our returns are completely correct.

[birchmd]

Done in #299