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

feat: add support for parsing emitted events #1227

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

3alpha
Copy link

@3alpha 3alpha commented Sep 13, 2024

Motivation and Resolution

provider.getEvents method returns list of EmittedEvents which contains fields for block number, block hash and transaction hash. Parsing list of those events with parseEvents would drop those fields. This PR adds them back to the returned array if they are available.

RPC version (if applicable)

...

Usage related changes

Users now use additional data from parsed events structure.

Development related changes

N/A

Checklist:

  • Performed a self-review of the code
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Linked the issues which this PR resolves
  • Documented the changes in code (API docs will be generated automatically)
  • Updated the tests
  • All tests are passing

@PhilippeR26
Copy link
Collaborator

Hello,
Seems you didn't created any issue for this subject.
What's your motivation? Bug resolution? improvement?
In which case will this modification be useful?

Copy link
Collaborator

@tabaktoni tabaktoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@tabaktoni
Copy link
Collaborator

Hello, Seems you didn't created any issue for this subject. What's your motivation? Bug resolution? improvement? In which case will this modification be useful?

It looks like we lost part of the information about events's block and tx after parsing the event.
As this part is added at the end of obj it should be backward compatible?

@3alpha
Copy link
Author

3alpha commented Sep 13, 2024

Hello, Seems you didn't created any issue for this subject. What's your motivation? Bug resolution? improvement? In which case will this modification be useful?

Hey, sorry, I can create issue if necessary. Issue this solves is situation where you get events via provider.getEvents method, you parse them and want to know, for example, who triggered transaction. Currently, data that connects events with "upsteam" structures like transactions and blocks gets dropped and this fixes that. With this fix, you can easily fetch transaction hash and from there fetch transaction and from there caller.

Only issue is how are these new keys ordered so backwards compatibility is not accidentally broken in code that uses Object.keys(event)[0] to fetch event names. I am pretty sure its by order of insertion, but I am not 100% sure.

@penovicp
Copy link
Collaborator

penovicp commented Sep 15, 2024

Hello, Seems you didn't created any issue for this subject. What's your motivation? Bug resolution? improvement? In which case will this modification be useful?

Hey, sorry, I can create issue if necessary. Issue this solves is situation where you get events via provider.getEvents method, you parse them and want to know, for example, who triggered transaction. Currently, data that connects events with "upsteam" structures like transactions and blocks gets dropped and this fixes that. With this fix, you can easily fetch transaction hash and from there fetch transaction and from there caller.

Only issue is how are these new keys ordered so backwards compatibility is not accidentally broken in code that uses Object.keys(event)[0] to fetch event names. I am pretty sure its by order of insertion, but I am not 100% sure.

The object key order used to be undefined, but when people kept relying on the JS engine behaviour that preserved the order it was eventually codified in the language specification. So non-numeric string keys, like the ones from this PR, should be ordered by their insertion order.

Copy link
Collaborator

@PhilippeR26 PhilippeR26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question.

@@ -194,57 +194,62 @@ function mergeAbiEvents(target: any, source: any): Object {
* ```
*/
export function parseEvents(
providerReceivedEvents: RPC.Event[],
providerReceivedEvents: RPC.EmittedEvent[] | RPC.Event[],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why RPC.EmittedEvent[] | RPC.Event[]?
Isn't it only RPC.EmittedEvent[]?
In which case are block_hash, block_number & transaction_hash not included?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block/tx info is not included in a case where you first fetch transaction receipt with starknet_getTransactionReceipt. In those kinds of reponses RPC.Event struct is returned (see: this). On the other hand when fetching data with starknet_fetchEvents returned struct is RPC.EmittedEvents.

So I thought it would be nice to leave that as is in case someone had code like this

events.parseEvents(receipts.map(t => t.events),...)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the root cause of this wrong input type of events.parseEvents() is due to Contract.parseEvent(). This method is calling events.parseEvents() with RPC.event format ; here is the root cause of all this mess.
I think that you have to modify Contract.parseEvent() :

  • if the event is included in a block, this type has to be sent to events.parseEvents() :
type EMITTED_EVENT = EVENT & {
  block_hash: BLOCK_HASH;
  block_number: BLOCK_NUMBER;
  transaction_hash: TXN_HASH;
};
  • if the event is in a PENDING transaction, this type has to be sent to events.parseEvents() :
type EMITTED_EVENT = EVENT & {
  transaction_hash: TXN_HASH;
};

events.parseEvents() has to accept both types as input.

RPC.Event[] type was here only to handle Contract.parseEvent() ; it's useless for users. They uses EVENTS_CHUNKtype (result of RpcProvider.getEvents()), that includes EmittedEvent type. Never RPC.Events.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how pending transaction have anything to do with this. AFAIK events are created and emitted during transaction execution. So there is always tx hash, block hash and block number corresponding to the event. They are missing from events array of the recipiet because that is always available in parent (receipt) object.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If PENDING is not appropriate, then we have still a problem in Contract.parseEvents() :

(receipt as InvokeTransactionReceiptResponse).events?.filter(

InvokeTransactionReceiptResponse type is not the good one, because it's made of INVOKE_TXN_RECEIPT | PENDING_INVOKE_TXN_RECEIPT
And in this case, only EMITTED_EVENT should be transfered from Contract.parseEvent() to events.parseEvents() (not RPC.event).
Comment about uselessness of RPC.event as input of events.parseEvents() remains valid.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understood correctly, you would like Contract.parseEvent() to change and look like this?

  public parseEvents(receipt: GetTransactionReceiptResponse): ParsedEvents {
    return parseRawEvents(
      (receipt as InvokeTransactionReceiptResponse).events?.
      map(event => { return {
        block_hash: (receipt as INVOKE_TXN_RECEIPT).block_hash,
        block_number: (receipt as INVOKE_TXN_RECEIPT).block_number,
        transaction_hash: (receipt as INVOKE_TXN_RECEIPT).transaction_hash,
        ...event
      }})
      .filter(
        (event) => cleanHex(event.from_address) === cleanHex(this.address),
        []
      ) || [],
      this.events,
      this.structs,
      CallData.getAbiEnum(this.abi)
    );
  }

Copy link
Collaborator

@PhilippeR26 PhilippeR26 Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this way.
I tested successfully this :
contract/default.ts :

public parseEvents(receipt: GetTransactionReceiptResponse): ParsedEvents {
let parsed: ParsedEvents;
receipt.match({
  success: (txR: SuccessfulTransactionReceiptResponse) => {
    const emittedEvents =
      (txR as InvokeTransactionReceiptResponse).events
        ?.map((event) => {
          return {
            block_hash: (txR as INVOKE_TXN_RECEIPT).block_hash,
            block_number: (txR as INVOKE_TXN_RECEIPT).block_number,
            transaction_hash: (txR as INVOKE_TXN_RECEIPT).transaction_hash,
            ...event,
          };
        })
        .filter((event) => cleanHex(event.from_address) === cleanHex(this.address), []) || [];
    parsed = parseRawEvents(
      emittedEvents,
      this.events,
      this.structs,
      CallData.getAbiEnum(this.abi)
    );
  },
  _: () => {
    throw Error('This transaction was not successful.');
  },
});
return parsed!;
}

events/index.ts :

export function parseEvents(
  providerReceivedEvents: RPC.EmittedEvent[],
  abiEvents: AbiEvents,
  abiStructs: AbiStructs,
  abiEnums: AbiEnums
): ParsedEvents {
...

@PhilippeR26
Copy link
Collaborator

I have implemented my last message, to finalize this PR.

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 this pull request may close these issues.

4 participants