Andrew7234/abi api

[Andrew7234]

After 540, we now parse tx data and event data using the abi we have available. The parsed fields are stored in several new columns which we now expose in the api.

Note: The abi-parsed event data is stored in the existing evm_log_params column, which requires no changes.

Note**: This PR renames the EvmEventParam type to EvmAbiParam, which is more accurate and also necessary because this type is also used by the runtime transaction evm_fn_params and txErr.params.

Note***: The e2e_regression tests don't change because we currently don't download the contract abi from ipfs during these tests.

» curl "http://localhost:8008/v1/sapphire/transactions/92d09287efdc88fff15c6cf63f3405529441ff719d2ea5f98ff52283753eccb4" | jq                andrewlow@Beleriand
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1780  100  1780    0     0   111k      0 --:--:-- --:--:-- --:--:--  158k
{
  "is_total_count_clipped": false,
  "total_count": 1,
  "transactions": [
    {
      "amount": "0",
      "body": {
        "address": "mYYzvfbuMqnMpsmiR/QoWW6OZdg=",
        "data": "nEW2bgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAKAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAIwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADtiYWZ5cmVpZmtybmJoZWhwdXNzY215bW5sdDd5dml0aGppdjRrZnF5ZjRjN3BqcTJ2em9lbnNrNGwzaQAAAAAA",
        "value": ""
      },
      "charged_fee": "11351000000000000",
      "eth_hash": "b087daa98fc02bf7c9357d44305cfc57a11a8900a70bb44f83ffef71fa872b21",
      "evm_fn_name": "acceptTaskResults",
      "evm_fn_params": [
        {
          "evm_type": "uint256[]",
          "name": "_taskIds",
          "value": [
            140
          ]
        },
        {
          "evm_type": "bytes",
          "name": "_proof",
          "value": ""
        },
        {
          "evm_type": "bytes",
          "name": "_report",
          "value": "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADtiYWZ5cmVpZmtybmJoZWhwdXNzY215bW5sdDd5dml0aGppdjRrZnF5ZjRjN3BqcTJ2em9lbnNrNGwzaQAAAAAA"
        }
      ],
      "fee": "1500000000000000000",
      "gas_limit": 15000000,
      "gas_used": 113510,
      "hash": "92d09287efdc88fff15c6cf63f3405529441ff719d2ea5f98ff52283753eccb4",
      "index": 0,
      "method": "evm.Call",
      "nonce_0": 146,
      "round": 410922,
      "sender_0": "oasis1qr5tj6pk8rm447u3k588a7hgkxrzus5vxqq3wsyq",
      "sender_0_eth": "0x7Df2e133FCFF612aA46995E82A54335F716B66be",
      "size": 558,
      "success": true,
      "timestamp": "2023-05-04T15:53:11-07:00",
      "to": "oasis1qpwe6h0hw6s0x4skujhznld065300p83wgxnvu0n",
      "to_eth": "0x998633BDF6eE32A9CcA6c9A247F428596e8e65d8"
    }
  ]
}

[mitjat]

Hey. Looking at this makes me feel like I might have missed some stuff in the precursor PR with the analyzer, #540 . If so, apologies :(. Hopefully it's just been a long day for me and I'm just misunderstanding/misreading stuff in this PR.

[mitjat]

IIRC technically the error is never (except at old heights) just a plain string; message is our stringification of a name-and-anonymous-param structure that's something like Error("this is my error message"). So in my mind, it doesn't make sense to expose message and params; message is just a human-friendly rendition of params (if there are any; or just the plain text instead; and also possibly a non-evm prefix stirng like "reverted:").
Or maybe we want to argue that rendering the message is tricky (especially with the "reverted: " prefix and whatnot) even when structured params are given, and that pre-rendering into message is helpful, but it's also helpful for people to be able to access the structured part? I'm doubtful about the latter, but not strongly opposed to it.

The prior behavior on parsing errors, see RawMessage vs Message (= parsed RawMessage) here:

nexus/analyzer/runtime/extract.go

Lines 73 to 77 in 6d23919

	// The raw error message returned by the node. Note that this may be null.
	// https://github.com/oasisprotocol/oasis-sdk/blob/fb741678585c04fdb413441f2bfba18aafbf98f3/client-sdk/go/types/transaction.go#L488-L492
	RawMessage *string
	// The human-readable error message parsed from RawMessage.
	Message *string

You can see how the latter is derived from the former, and there are also unit tests.

Would it make sense for the backfilling parser to reuse more of the non-backfilling parser code? E.g. this function:

nexus/analyzer/runtime/extract.go

Line 676 in 6d23919

func tryParseErrorMessage(errorModule string, errorCode uint32, msg string) *string {

It currently only attempts to parse with a single ABI, Error(string), see line 698. The func would have to made a little more generic so the backfilling parser can pass in a custom ABI. Then we could use it in the backfilling analyzer.

At this point, I think I might be missing something.

nexus/analyzer/evmabibackfill/evm_abi_backfill.go

Lines 162 to 165 in 6d23919

	// See the docstring of tryParseErrorMessage in analyzer/runtime/extract.go
	// for more info.
	func cleanTxRevertReason(raw string) ([]byte, error) {
	s := strings.TrimPrefix(raw, "reverted: ")

even mentions tryParseErrorMessage() (which I'm trying to get us to reuse here) but then does not call it. Maybe we can clarify in a chat or call?

[Andrew7234]

expose message and params; message

Hm the txErr.Message field stores the error message for all types of tx errors. Non evm revert errors, such as execution failed: out of gas, are still returned as plaintext even in the most recent upgrade, so we can't remove the message field and replace it with params.

Agreed that the repetition between message and params for newer evm reverted txs is somewhat messy. We could avoid this by not populating params when we specifically see that the abi error has the format Error(string), but imo it's not worth special-casing this.

Edit**: Actually it looks like the Error(string) type is not included by default in the abi due to quirks with evm. reverted("my_error") is automatically encoded as the Error(string) type but isn't included in the abi. The backfilling analyzer shouldn't be able to parse such errors since they're not explicitly defined in the abi. Therefore we should continue optimistically trying to parse tx evm reverted errors as Error(string). One caveat: some contracts may explicitly define the type, which would lead to the error message being replicated in message and params, but we have no control over this and it should be rare.

Would it make sense for the backfilling parser to reuse more of the non-backfilling parser code?

Yeah the explicit abi logic is a remnant from the days before Warren added the abiparse package. We should define a bare-bones abi struct with an Error(string) error and update tryParseErrorMessage to use the new abiparse.ParseError(err, abi) function. I'd prefer to do this in a follow-up PR; filed a ticket to track: #776

[mitjat]

Thank you for the explanation and for the ticket! SGTM to do it in a separate PR.

This makes sense. I think my confusion was because I forgot that message is for all types of tx errors, and params was specific to EVM-contract-generated errors. Maybe we can say something to that effect in the docstrings? It's easy to get lost between all the layers :/
E.g. in message:

This field, like `code` and `module`, can represent an error that originated anywhere in the paratime, i.e. either inside or outside a smart contract.

A common special case worth calling out: When the paratime is EVM-compatible (e.g. Emerald or Sapphire) and the error originates inside a smart contract (using `revert` in solidity), the following will be true:
- `module` will be "evm" and `code` will be 8; see [here](https://github.com/oasisprotocol/oasis-sdk/blob/runtime-sdk/v0.8.3/runtime-sdk/modules/evm/src/lib.rs#L128) for other possible errors in the `evm` module.
- `message` will contain the revert reason, either in its raw ABI-encoded form, or its best-effort human-readable rendition.

In params, you technically say the thing already ("Only present for errors from verified contracts."), but WDYT about breaking it down and instead separately calling out "from contracts" and "verified contracts"? E.g.

Present only when
- the error originated from within a smart contract (e.g. via `revert` in Solidity), and 
- the contract is verified or the revert reason is a plain String.
If this field is present, `message` will include a human-readable representation of it.

Feel free to reword, obviously. I just tried to cover the points I find important.

[Andrew7234]

Updated, thanks!

message will contain the revert reason, either in its raw ABI-encoded form

We currently do not return the raw abi-encoded message; we only store it in runtime_transactions.error_message_raw. We could easily change the api to return the raw abi-encoded message if no parsed message is found, though I'm not sure how useful that would be for most users. Perhaps the main use case would be for contract devs who have the abi and are able to decode it themselves?

[mitjat]

Getting there!
I only left 1 long comment (that will hopefully just require to read+copy+paste) and 1 short but meaty comment about not tracking the error type. Other than that, LGTM!

[mitjat]

Thank you for the explanation and for the ticket! SGTM to do it in a separate PR.

This makes sense. I think my confusion was because I forgot that message is for all types of tx errors, and params was specific to EVM-contract-generated errors. Maybe we can say something to that effect in the docstrings? It's easy to get lost between all the layers :/
E.g. in message:

This field, like `code` and `module`, can represent an error that originated anywhere in the paratime, i.e. either inside or outside a smart contract.

A common special case worth calling out: When the paratime is EVM-compatible (e.g. Emerald or Sapphire) and the error originates inside a smart contract (using `revert` in solidity), the following will be true:
- `module` will be "evm" and `code` will be 8; see [here](https://github.com/oasisprotocol/oasis-sdk/blob/runtime-sdk/v0.8.3/runtime-sdk/modules/evm/src/lib.rs#L128) for other possible errors in the `evm` module.
- `message` will contain the revert reason, either in its raw ABI-encoded form, or its best-effort human-readable rendition.

In params, you technically say the thing already ("Only present for errors from verified contracts."), but WDYT about breaking it down and instead separately calling out "from contracts" and "verified contracts"? E.g.

Present only when
- the error originated from within a smart contract (e.g. via `revert` in Solidity), and 
- the contract is verified or the revert reason is a plain String.
If this field is present, `message` will include a human-readable representation of it.

Feel free to reword, obviously. I just tried to cover the points I find important.

[mitjat]

After the discussion below: I'm open to renaming this to revert_params. It's more specific and descriptive, and it's accurate for current usage. There is a small risk that we might want to expand the use of the field to non-revert scenarios later, though I don't see it. Your call.

[Andrew7234]

sounds good, updated!