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.

Sign up for GitHub

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

Update default query for zkapp fetch actions to not add params #1794

Merged
merged 3 commits into from
Aug 29, 2024
Merged

Update default query for zkapp fetch actions to not add params #1794

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef13fa1
update default query for zkapp fetch actions to not add params
45930 Aug 20, 2024
a82cd12
Merge branch 'main' into fetch-events-timeout-fix
45930 Aug 28, 2024
82dcbe1
updating error message on no events defined case
45930 Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions src/lib/mina/zkapp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
AccountUpdateLayout,
AccountUpdateTree,
} from './account-update.js';
import type { EventActionFilterOptions } from './graphql.js';
import {
cloneCircuitValue,
FlexibleProvablePure,
Expand Down Expand Up @@ -1058,21 +1059,18 @@ super.init();
chainStatus: string;
}[]
> {
const queryFilterOptions: EventActionFilterOptions = {};
if(start.greaterThan(UInt32.from(0)).toBoolean()) {
queryFilterOptions.from = start;
}
if(end) {
queryFilterOptions.to = end;
}
// filters all elements so that they are within the given range
// only returns { type: "", event: [] } in a flat format
let events = (
await Mina.fetchEvents(this.address, this.self.body.tokenId, {
from: start,
to: end,
})
await Mina.fetchEvents(this.address, this.self.body.tokenId, queryFilterOptions)
)
.filter((eventData) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

so, good to remove

let height = UInt32.from(eventData.blockHeight);
return end === undefined
? start.lessThanOrEqual(height).toBoolean()
: start.lessThanOrEqual(height).toBoolean() &&
height.lessThanOrEqual(end).toBoolean();
})
.map((event) => {
return event.events.map((eventData) => {
let { events, ...rest } = event;
Expand All @@ -1088,8 +1086,23 @@ super.init();
let sortedEventTypes = Object.keys(this.events).sort();

return events.map((eventData) => {
// If we don't know what types of events this zkapp may emit, then return the raw data
if (sortedEventTypes.length === 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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();

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

beautiful!

return {
...eventData,
type: "Object",
event: {
data: eventData.event.data,
transactionInfo: {
transactionHash: eventData.event.transactionInfo.hash,
transactionStatus: eventData.event.transactionInfo.status,
transactionMemo: eventData.event.transactionInfo.memo,
},
},
};
}
// if there is only one event type, the event structure has no index and can directly be matched to the event type
if (sortedEventTypes.length === 1) {
else if (sortedEventTypes.length === 1) {
let type = sortedEventTypes[0];
let event = this.events[type].fromFields(
eventData.event.data.map((f: string) => Field(f))
Expand Down