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

Emit events for all transfers #88

Merged
merged 3 commits into from
Jul 15, 2024

Conversation

kantp
Copy link
Collaborator

@kantp kantp commented Jul 8, 2024

No description provided.

@kantp kantp linked an issue Jul 8, 2024 that may be closed by this pull request
The `TransferEvent` was not expressive enough to capture all transfers, only two party a -> b
transfers. Furthermore, even a transfer of that type would not emit an event unless it was initiated
using `FungibleToken.transfer()`.

We now emit individual events per `AccountUpdate` that's using the token.
@kantp kantp force-pushed the kantp/87-properly-emit-events-for-all-transfers branch from 5b8b08c to 1bdd370 Compare July 11, 2024 14:55
@kantp kantp marked this pull request as ready for review July 11, 2024 14:55
FungibleToken.ts Outdated
@@ -169,6 +168,12 @@ export class FungibleToken extends TokenContract {
let totalBalance = Int64.from(0)
this.forEachUpdate(updates, (update, usesToken) => {
this.checkPermissionsUpdate(update)
if (usesToken) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can't use if (usesToken), it's not a boolean but a Bool (o1js class) and is always truthy

doing things conditionally in a circuit typically involves Provable.if() or some form of option type

Copy link
Collaborator

Choose a reason for hiding this comment

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

we will have to implement a conditional version of this.emitEvent()

Copy link
Collaborator

Choose a reason for hiding this comment

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

ultimately, emitEvent() just updates this.body.events (which is a hash) and the conditional version does the same but with Provable.if()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I pushed a change that assembles an array of Option(BalanceChangeEvent), then filters our the none() values, and emits an event for those remaining.

Is there a simpler way of doing this, or is that how I should do it?

@kantp kantp changed the title WIP: Emit events for all transfers Emit events for all transfers Jul 12, 2024
FungibleToken.ts Outdated
value: Option<BalanceChangeEvent>,
_index: number,
_array: Option<BalanceChangeEvent>[],
) => value.isSome)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same issue as before, we can't filter on value.isSome, it's a Bool, always truthy

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, right!

Is there an equivalent function to filter on a Bool?

@kantp kantp added this pull request to the merge queue Jul 15, 2024
Merged via the queue into main with commit 99f0623 Jul 15, 2024
3 checks passed
@kantp kantp deleted the kantp/87-properly-emit-events-for-all-transfers branch July 15, 2024 12:20
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.

Properly emit events for all transfers
2 participants