Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Extending the ethereum.Transaction to contain all logs #3218

Closed
schmidsi opened this issue Feb 3, 2022 · 9 comments · Fixed by #3373
Closed

Feature request: Extending the ethereum.Transaction to contain all logs #3218

schmidsi opened this issue Feb 3, 2022 · 9 comments · Fixed by #3373
Assignees

Comments

@schmidsi
Copy link
Contributor

schmidsi commented Feb 3, 2022

What is the current behavior?
Currently, the ethereum.Transaction type only exposes the following values:

  /**
   * An Ethereum transaction.
   */
  export class Transaction {
    constructor(
      public hash: Bytes,
      public index: BigInt,
      public from: Address,
      public to: Address | null,
      public value: BigInt,
      public gasLimit: BigInt,
      public gasPrice: BigInt,
      public input: Bytes,
      public nonce: BigInt,
    ) {}
  }

It would be great if there would also be a possibility to also access all the logs. Something like:

  /**
   * An Ethereum transaction.
   */
  export class Transaction {
    constructor(
      public hash: Bytes,
      public index: BigInt,
      public from: Address,
      public to: Address | null,
      public value: BigInt,
      public gasLimit: BigInt,
      public gasPrice: BigInt,
      public input: Bytes,
      public nonce: BigInt,
      public events: Event[] // To contain all events
    ) {}
  }

This would for example enable the possibility to write a subgraph that tracks the actual sale price of NFTs by looking at the ERC20 token Transfers from buyer to seller.

I know that the Firehose will enable this but adding this issue here for tracking since a lot of folks are requesting this feature.

Prior art:

@azf20
Copy link
Contributor

azf20 commented Feb 24, 2022

Thanks @schmidsi!

Discussed with @tilacog, this information is available on the transaction receipt.

I think we would want to fetch this information conditionally (based on the needs of the subgraph developer), so maybe we could introduce a new function ethereum.Transaction.getReceipt() which returns a Receipt. This would include the other logs from that receipt, and other information too:

transactionHash : DATA, 32 Bytes - hash of the transaction.
transactionIndex: QUANTITY - integer of the transactions index position in the block.
blockHash: DATA, 32 Bytes - hash of the block where this transaction was in.
blockNumber: QUANTITY - block number where this transaction was in.
from: DATA, 20 Bytes - address of the sender.
to: DATA, 20 Bytes - address of the receiver. null when its a contract creation transaction.
cumulativeGasUsed : QUANTITY  - The total amount of gas used when this transaction was executed in the block.
gasUsed : QUANTITY  - The amount of gas used by this specific transaction alone.
contractAddress : DATA, 20 Bytes - The contract address created, if the transaction was a contract creation, otherwise null.
logs: Array - Array of log objects, which this transaction generated.
logsBloom: DATA, 256 Bytes - Bloom filter for light clients to quickly retrieve related logs.

gasUsed in particular has been requested before. This Receipt could either be fetched from the Block cache (if the block is cached, and has receipts), or it could be fetched from the RPC (the Ethereum provider is accessible from the runtime). This logic already exists in graph-node (while checking calls were not part of failed transactions)

This would require a new apiVersion.

I believe that if this is quite a "quick win" there is value in supporting receipts with RPC indexing (rather than waiting for the Firehose, as not all EVM chains will have firehose support immediate).

Interested in your thoughts @leoyvens @Jannis @tilacog

@schmidsi
Copy link
Contributor Author

schmidsi commented Feb 24, 2022

One question that comes to my mind would be how to parse the logs field, as they are encoded by default. I'm not sure if we have the ABI-decoding functionality available in @graphprotocol/graph-ts. We need to somehow load the ABI first and then parse the logs.

@azf20
Copy link
Contributor

azf20 commented Feb 24, 2022

Yes - I think we can leverage the ABIs that we already have (which are used currently for eth_call), and we have the encoding / decoding ABI functionality. I think at the outset the decoding can happen in the Assemblyscript? (indeed there might be logs which can't be decoded if they're not from the expected contracts). We will need to ensure that that pattern works of course, but I think the raw tools are there

@leoyvens
Copy link
Collaborator

Making this a host fn is more flexible, but for performance it would be best for this to be declared in the manifest so it can be fetched in the block stream. This also fits more naturally firehose, which streams the data according to the manifest. @fordN once implemented this for block handlers in #1761.

@tilacog
Copy link
Contributor

tilacog commented Feb 25, 2022

Should we aim at resuming Ford's previous work, or would it be better to implement access to receipts via a host function?

I agree with @leoyvens that the former seems to improve the current design and can be a more strategic move in the long term.

Considering Ford's PR, maybe we could promote the manifest's block input type to be at the mapping level instead of only being available to blockHandlers?

@azf20
Copy link
Contributor

azf20 commented Feb 25, 2022

Agree that this could be specified in the manifest, but I think the priority here is to fetch the receipt for the specific transaction which emitted the event, rather than all the receipts in the block - if they're not in the cache, that could be a lot of unnecessary rpc calls.
It will be easy to support both with the firehose, as firehose blocks have all this information

@leoyvens
Copy link
Collaborator

It wasn't clear to me from the OP if the use case is for all receipts in a block or for the receipts associated to an event's transaction. Still that doesn't mean it must be a host fn, the event handler in the manifest could specify that it needs all receipts for its transaction.

Our block cache structure assumes that either all receipts for a given block are cached, or none are. Having it support caching receipts for individual transactions is tricky, introducing a separate cache might be the safest path.

@tilacog
Copy link
Contributor

tilacog commented Mar 1, 2022

We have agreed on using the new receipt manifest key

eventHandlers:
  event: Transfer(address,address,uint256)
  handler: myEventHandler
  receipt: true

Where receipt == true the ethereum.event passed to the event handler would include an optional receipt:

public receipt: Receipt | null,

Having said so, here is the proposed roadmap to implement this feature concerning graph-node:

  • Define a new AscReceipt type to hold transaction receipt data.
  • Define a new version for the AscEvent type that holds a Option<AscReceipt> value.
  • Conditionally provision the runtime with the right AscEvent type based on the new manifest specs.
  • Introduce specVersion 0.0.5 that will accept the eventHandlers.receipt field in the manifest file.
  • Introduce apiVersion 0.0.7 that will determine which AscEvent type to use in the runtime.

We understand that having a transaction receipt cache is also desired, but it is out of the scope of this task. We could track its discussion and development in a separate issue.

@schmidsi
Copy link
Contributor Author

schmidsi commented Mar 8, 2022

To answer @leoyvens question:
| It wasn't clear to me from the OP if the use case is for all receipts in a block or for the receipts associated to an event's transaction.

It is about to retrieve all receipts associated to an event's transaction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants