Add channelId argument to AssetTransferred event #595
Conversation
snario
force-pushed
the
liam-add-origin
branch
2 times, most recently
from
78485e3 to
cc9da61
Compare
|
Error in CI is related to #602 |
snario
force-pushed
the
liam-add-origin
branch
from
cc9da61 to
1b60f6c
Compare
| assetHolderAddress: string, | ||
| origin: string, | ||
| // @ts-ignore | ||
| destination: string, // TODO: Should we take this into account? |
There was a problem hiding this comment.
It's possible in future that destination is removed so that it is not even part of the data emitted in the event. We might decide that it is redundant information. It was introduced mainly to make testing easier, but we could get the data from somewhere else most likely. In the case of an ERC20, for example, the Token contract itself will emit a Transfer event when the Tokens are transferred.
| _transferAsset(_bytes32ToAddress(allocation[m].destination), payoutAmount); | ||
| emit AssetTransferred(allocation[m].destination, payoutAmount); | ||
| emit AssetTransferred(channelId, allocation[m].destination, payoutAmount); | ||
| } else { | ||
| holdings[allocation[m].destination] += payoutAmount; | ||
| } |
There was a problem hiding this comment.
My comments above are because the event is only emitted inside a _isExternalDestination block. Do you think we ought to also emit an event when the holdings are updated? This could be the same event or a new one (with some appropriate name(s)). As it is now, no events will be emitted during e.g. a transferAll on a ledger channel that only allocates to other channels. The holdings mapping is public, however: but clients won't have a "trigger" to remind them to inspect it.
There was a problem hiding this comment.
We should make a decision on this at standup; it's not clear to me that it is important information for the wallet to keep track of.
snario
force-pushed
the
liam-add-origin
branch
2 times, most recently
from
84f1cd9 to
1325c09
Compare
snario
changed the title
snario
force-pushed
the
liam-add-origin
branch
from
1325c09 to
9f9a65e
Compare
| @@ -56,9 +56,12 @@ export interface DisplayMessageSent { | |||
|
|
|||
| export interface AssetTransferredEvent { | |||
| type: "WALLET.ASSET_HOLDER.ASSET_TRANSFERRED"; | |||
| assetHolderAddress: string; | |||
| channelId: string; | |||
|
|
|||
| // This is either a `channelId` or an external destination (both bytes32). | |||
There was a problem hiding this comment.
| // This is either a `channelId` or an external destination (both bytes32). | |
| // This is either a `channelId` or an external destination (both bytes32): |
No description provided.