Update default query for zkapp fetch actions to not add params

[45930]

Summary

DFST reported that fetching events for a given account was working via the fetch module, but not via the built-in zkapp.fetch method.

I discovered that the zkapp version always adds the from: 0 parameter, which I believe is executing a poorly optimized query on the archive node. I brought this issue up with the node operators team for further review.

This PR removes the from: 0 parameter by default and only adds parameters to the query when explicitly provided by the user.

Context

#1777

[45930]

Removing this filter because it is a duplicate of the filter applied by the archive node. Was it intentional to leave this as insurance?

[mitschabaude]

yeah, this implementation was written when the archive node API was still in an early WIP phase and simply always returned the full list of events

[mitschabaude]

so, good to remove

[45930]

There may be something I don't understand here. I ran an example script with an initialized smart contract and got the events payload from the graphQL query, then the script failed here because this.events was equal to {}, and there is no case to support that in this branching return.

How does a smart contract get its this.events hydrated? Does that happen in a compile step or something?

[Trivo25]

It does it right here

o1js/src/lib/mina/zkapp.ts

Line 1019 in a8df62c

let eventTypes: (keyof this['events'])[] = Object.keys(this.events);

o1js/src/lib/mina/zkapp.ts

Line 1121 in a8df62c

let sortedEventTypes = Object.keys(this.events).sort();

[mitschabaude]

this.events is set in the user code

class Contract extends SmartContract {
   events = { ... }  // HERE (note: this is executed when a new instance is created)
}

I think you could also just throw an error in this function if the events property wasn't changed, I don't see what zkapp.fetchEvents() gives us over Mina.fetchEvents() in that case

Anyway, if you keep this case @45930, it would be nice to have it share most of the code with the === 1 case

[mitschabaude]

The point of having SmartContract-specific methods instead of generic Mina methods is usually to make use of the custom type layout that a SC defines, for example for 8-field state and here for events

[45930]

@mitschabaude so in my example, I am using the mina NFT contract and this logs out {}. A new instance of a smart contract with an empty events property.

const zkApp = new NFTContractV2(address);
console.log(zkApp.events);

But the smart contract does emit events, e.g.

    @method async setOracle(oracle: PublicKey) {
      this.oracle.set(oracle);
      this.emitEvent("oracle", oracle);
    }
  
    @method async setPriceLimit(limit: UInt64) {
      this.priceLimit.set(limit);
      this.emitEvent("limit", limit);
    }

Though the developer did not define the events property, the contract may still emit events, so I would expect an instance of it to be able to fetch its own events. If the user must set this value, or we recommend to use Mina.fetchEvents() instead, then I think we still need a length == 0 case to raise a helpful error explaining the problem.

The original code checks for the length == 1 case, otherwise assumes that length > 1, so I got the error: Error fetching events TypeError: Cannot read properties of undefined (reading 'fromFields'), which makes no sense unless you understand that we're trying to access a lookup value that doesn't exist.

[mitschabaude]

we should probably make this.emitEvent() throw an error if there are no events defined! the event ids won't be set properly

[45930]

we should probably make this.emitEvent() throw an error if there are no events defined!

Ah, we already do this, so that makes the decision easier. I only got myself into this state by transcribing a smart contract incorrectly, and not including the events definition. I will update this case to throw an error instead so that it matches, but still gives a better error message than the Cannot read properties of undefined one in case someone else makes the same mistake.

[45930]

New error message would be:

Error: fetchEvents: You are trying to fetch events without having declared the types of your events.
Make sure to add a property `events` on NFTContractV2, for example: 
class NFTContractV2 extends SmartContract {
  events = { 'my-event': Field }
}
Or, if you want to access the events from the zkapp account B62qs2NthDuxAT94tTFg6MtuaP1gaBxTZyNv9D3uQiQciy1VsaimNFT without casting their types
then try Mina.fetchEvents('B62qs2NthDuxAT94tTFg6MtuaP1gaBxTZyNv9D3uQiQciy1VsaimNFT') instead.

[45930]

@mitschabaude do the changes in 82dcbe1 look good to you?

[mitschabaude]

beautiful!

[Trivo25]

It does it right here

o1js/src/lib/mina/zkapp.ts

Line 1019 in a8df62c

let eventTypes: (keyof this['events'])[] = Object.keys(this.events);

o1js/src/lib/mina/zkapp.ts

Line 1121 in a8df62c

let sortedEventTypes = Object.keys(this.events).sort();