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

fetchEvents for LocalBlockchain #323

Merged
merged 8 commits into from
Aug 4, 2022
Merged

Conversation

Trivo25
Copy link
Member

@Trivo25 Trivo25 commented Aug 3, 2022

Description

This is the first part of extending LocalBlockchain to support fetchEvents and later in a second PR fetchSequenceEvents.

Under the hood

A smart contract can emit events

@method emitMe(value: Field) {
	this.emit("someEvent", value);
}

but the LocalBlockchain did not store emitted events until now. After the transaction has successfully been applied to the Ledger via applyJsonTransaction, all events are stored in a variable events. Events are identified by publicKey and tokenId.

LocalBlockchain now exposes a closure fetchEvents(), which returns all events that have been emitted.

The SmartContract class has been extended by adding a new method fetchEvents(start: UInt32, end?: UInt32): { type: string; event: AsFieldElements<any> }[], which returns a list of all events that have been emitted by a SmartContract instance, using the tokenId and publicKey as identifier and start and end as range options.

Note: There has been a discussion regarding what type to use in order to filter events. I went with using slots as an indicator for now.

Example

Developers can now call the fetchEvents(start: UInt32, end?: UInt32) method on an instance of a smart contract that uses the LocalBlockchain as active instance.

class MyApp extends SmartContract {
	
	events = {
		someEvent: Field,
	};

	@method emitMe() {
		this.emit("someEvent", Field.zero);
	}
}

// ...

let events = myAppInstance.fetchEvents(UInt32.from(0))
console.log(events)
/*
[
	{ type: "someEvent", event: Field(0)  }
]

*/

// used to match field values back to their original type
let sortedEventTypes = Object.keys(this.events).sort();

return events.map((event: any) => {
Copy link
Member Author

@Trivo25 Trivo25 Aug 3, 2022

Choose a reason for hiding this comment

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

It would probably be smarter to set new events at the beginning of the array instead of the end (reverse). How is it enforced on the protocol level/archive api?

Copy link
Member

Choose a reason for hiding this comment

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

hmm, not sure what's better to use 🤔 yeah we should probably mirror whatever the graphql API ends up doing

@Trivo25 Trivo25 marked this pull request as ready for review August 3, 2022 08:18
@Trivo25 Trivo25 requested review from bkase, mitschabaude, MartinMinkov and ymekuria and removed request for bkase and mitschabaude August 3, 2022 08:18
@Trivo25 Trivo25 changed the title fetchEvents to LocalBlockchain fetchEvents for LocalBlockchain Aug 3, 2022
Copy link
Member

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

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

nice work on your first PR! left a couple of suggestions

src/lib/mina.ts Outdated Show resolved Hide resolved
src/lib/mina.ts Outdated Show resolved Hide resolved
src/lib/zkapp.ts Outdated Show resolved Hide resolved
src/lib/zkapp.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ymekuria ymekuria left a comment

Choose a reason for hiding this comment

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

Nice work Florian! I left some comments.

@prop pub: PublicKey;
@prop value: Field;

constructor(pub: PublicKey, value: Field) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We had a discussion awhile back about adding a static from method that invokes the original constructor instead of overriding the constructor in examples to encourage this pattern. Here is the discussion for background and here is an example.

What you are doing works (passing constructor arguments to super) but we want to encourage the .from pattern to the community. @mitschabaude do you still think we should encourage this .from pattern in our examples?

Copy link
Member

Choose a reason for hiding this comment

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

Uhm, to be honest, I'm not sure. I think it's fine either way. My original idea was that you'd have to do nothing and use the generic constructor. .from would've only been for the few cases where you'd want to transform the input arguments. However, by now I think a constructor like this, which is a no-op, is still nice to add, because it becomes properly typed

Copy link
Contributor

Choose a reason for hiding this comment

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

That works for me. I'm fine leaving it as is if we are in agreement.

src/lib/mina.ts Outdated
): Promise<any[]> {
let eventsForAccount =
events?.[publicKey.toBase58()]?.[Ledger.fieldToBase58(tokenId)];
return eventsForAccount === undefined ? [] : eventsForAccount;
Copy link
Member

Choose a reason for hiding this comment

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

return events?.[publicKey.toBase58()]?.[Ledger.fieldToBase58(tokenId)] ?? []

😉

Copy link
Member Author

Choose a reason for hiding this comment

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

ahh, perfect

Copy link
Contributor

@MartinMinkov MartinMinkov left a comment

Choose a reason for hiding this comment

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

Nothing to add that others haven't said for feedback, approving preemptively!
Great job! :D

@Trivo25 Trivo25 merged commit e667f03 into main Aug 4, 2022
@Trivo25 Trivo25 deleted the feature/localBlockchain-events branch August 4, 2022 09:51
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.

None yet

4 participants