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

Move Withdrawn event to Vault and add balanceAfter params #158

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

joshuahannan
Copy link
Member

@joshuahannan joshuahannan commented Apr 19, 2024

Description

The Vault interface is the more logical place to emit the Withdrawn and Deposited events because the Vault is always the starting and final destination for fungible tokens, and provides us with more metadata to emit in the event.

This PR updates the FungibleToken standard to emit Withdrawn from FungibleToken.Vault.withdraw() instead of FungibleToken.Provider.withdraw(). It also adds balanceAfter parameters to both events to show the balance of the Vault after the operation has occurred.


For contributor use:

  • Targeted PR against v2-standard branch
  • Code follows the standards mentioned here.
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

Copy link
Contributor

@sisyphusSmiling sisyphusSmiling left a comment

Choose a reason for hiding this comment

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

I like this in principle - feels cleaner and makes sense. But I'm curious if it impacts existing products/platforms. For example, a scope provider might expect the event to contain the scoped resource from address but that event value will now be the underlying vault's address (assuming they're distinct addresses).

This is a small detail, and it may not actually be a big deal, but I can see how the difference in expectations could make an outsized difference. I'd be interested in hearing from app/platform builders.

@joshuahannan
Copy link
Member Author

@sisyphusSmiling Bjarte and I talked for a little about this and he is totally on board with it. He was the one who suggested it actually! We figure that if provider implementations want to surface information about their token movements, then they should define custom events to do that. I think it is safer to have both Deposit and Withdraw events show information about token movements in and out of the same place.

Copy link
Contributor

@sisyphusSmiling sisyphusSmiling left a comment

Choose a reason for hiding this comment

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

Ah alright, that's great! I really like that the events can now include the updated balances.

contracts/FungibleToken.cdc Outdated Show resolved Hide resolved
contracts/FungibleToken.cdc Outdated Show resolved Hide resolved
@bluesign
Copy link

Doesn't this miss some events? if I just implement provider or receiver, they will not emit events it seems.

@bjartek
Copy link
Contributor

bjartek commented Apr 20, 2024

Previously events where never emitted when you only implemented some of thr interfaces.

It will miss them in this case, but things like TokenForwarding already have a good events.

So for me the trade off is worth it. The guidelines just need to say that if you do this you have to emit your own events.

@joshuahannan joshuahannan merged commit b518689 into v2-standard Apr 24, 2024
2 checks passed
@joshuahannan joshuahannan deleted the modify-events branch April 24, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants