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 deposit and withdrawal events #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

devrandom
Copy link

Emit events for efficient monitoring of the blockchain for our transactions.

@ligi
Copy link

ligi commented Sep 26, 2019

Would love if this (or something like this) could be merged! @christianlundkvist what do you think? Sure it makes it a little bit less simple - but the gain for the minimal complication is high. E.g. it makes it possible for wallets to list all transactions for the wallet in a decentralized manner.
The only thing I would change is the naming to Executed and Received and add data to executed.
Basically like this implementation: https://github.com/argentlabs/argent-contracts/blob/develop/contracts/MultiSigWallet.sol#L25

@nmushegian
Copy link

nmushegian commented Nov 14, 2019

IMO it should log everything except the signature data, and index the addresses.

address indexed destination, uint value, bytes data, address indexed executor, uint gasLimit

or just

address address caller, address address target, uint gas, bytes data

I agree with @ligi that "Deposit" and "Withdrawal" are not great names. The most common "Withdrawal" is likely to have 0 ETH value, just calling other contracts. I would call them Received and Executed as well.

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.

4 participants