Removes pre-condition for mint, adds conditionals for events

[joshuahannan]

I am curious to hear thoughts on this proposal. It seems to me that nobody really cares about events coming from fungible token contracts that don't include the address of the account that owns them, so I though that it might help to just not emit them if the owner field isn't set. I also figured that if the amount of tokens being withdrawn or deposited is zero, we could also skip emitting the event.

My thoughts are that if the vault isn't stored in storage anywhere, then the event is kind of meaningless, so it saves space and execution time to just not emit it, such as when we are paying rewards or moving tokens in the staking contract. We have equivalent events defined in the staking contract that are much more useful than any deposit or withdrawal events because they include the node ID or delegator ID.

If it wasn't already clear, I would want to apply these changes to the FlowToken contract also

(I also modified a few dependencies to get CI to pass, but those aren't related)

[rheaplex]

This is a tricky one.

If these changes were implemented it's a shame we can't change the Address? in these events to Address, as that would be a nice way both of signalling that the Address will always be set and of (trivially) reducing the off-chain code required to process the events.

Removing the > 0 precondition from deposit() does mean that we can now get zero value deposits, and combining those with deposits to (temporary or newly created) Vaults that do not have a set address means we will obviously get events that contain (0, nil) as their data. Which doesn't seem very informative. But there are edge cases where it may be, and where the advantages of (a petty 😉) consistency win out for offchain observers over saving gas.

We won't always get a deposit following a withdrawal, however, as apart from anything else a Vault can always be saved to storage directly. So we would be reducing the number of cases in which events are emitted further rather than deviating from perfect consistency in withdraw/deposit pairs.

It may be the case that we should redesign the FT events to only emit some sort of exchange event where we have two addresses and a transfer between them, enforced in some way via an updated FT API. This would mean that offchain watchers are guaranteed to see only balanced withdrawal/deposits and can reason about them more simply. But it would lead to even fewer informative events, so maybe not. 😺

A good person to comment on this would be @orodio .

[bluesign]

It seems to me that nobody really cares about events coming from fungible token contracts that don't include the address of the account that owns them, so I though that it might help to just not emit them if the owner field isn't set.

Don't want to be too pessimistic but isn't this how we end up with 'buns without sesame' ? [0]

[0] https://www.joelonsoftware.com/2007/09/11/theres-no-place-like-127001/

[joshuahannan]

@bluesign you might be right. I just wanted to get some feedback about this just in case. I'm not super attached to it, but think it would be good. And I wouldn't want to go any farther than this. Can you think of a good reason not to do it?

[bluesign]

@joshuahannan In this current situation, actually this is correct fix. But my point was about the bigger picture a bit. Maybe the main problem is having owner = nil where we should have the owner. Maybe we need some fixes on Cadence level about ownership. I feel like owner=nil should not happen, if we can prevent. ( I don't remember exactly, but if I get vault from storage I think owner sets to nil, doesn't feel correct to me )

[rheaplex]

I feel like owner=nil should not happen, if we can prevent. ( I don't remember exactly, but if I get vault from storage I think owner sets to nil, doesn't feel correct to me )

The problem is that once a Vault is removed from storage in a transaction, we do not know which account it is associated with. Even without resources from multiple AuthAccounts in a single transaction, a Vault's association with the storage of the AuthAccount that it has just been removed from may not be strong enough to warrant identifying it as the Vault's owner. If we regard the owner as the storage that the Vault is in, this becomes tautological.

It may be the case that borrowed Vaults and other resources should act differently. But resources that have been removed from storage and retain an owner address may be conceptually equivalent to dangling pointers in ways we would need to think through.