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

Closes #133: Added events + required refactoring #135

Merged
merged 6 commits into from
Oct 28, 2019

Conversation

rmeissner
Copy link
Member

@rmeissner rmeissner commented Oct 15, 2019

Closes #133

This PR adds events for all kinds of interactions with the Safe. This will make it easier for 3rd party clients to monitor Safe instances.

The additional costs are between 2% when executing a transaction. For incoming transactions there is an additional 1k required.

Current event definitions (for a better overview):

    event ApproveHash(
        bytes32 indexed approvedHash,
        address indexed owner
    );
    event SignMsg(
        bytes32 indexed msgHash
    );
    event ExecutionFailure(
        bytes32 txHash, uint256 payment
    );
    event ExecutionSuccess(
        bytes32 txHash, uint256 payment
    );

    event ExecutionFromModuleSuccess(
        address indexed module
    );
    event ExecutionFromModuleFailure(
        address indexed module
    );

    event IncomingTransaction(
        address from, uint256 value
    );

@rmeissner rmeissner requested a review from Uxio0 October 15, 2019 13:53
@rmeissner rmeissner self-assigned this Oct 15, 2019
@rmeissner
Copy link
Member Author

rmeissner commented Oct 15, 2019

Alternative to the Execution and ExecutionPayment events (to save some gas):

  • Execution(bytes32 txHash, bool success, uint256 payment)
    • Transaction information can be parsed from the transaction where the event was thrown
    • Only edge case is when an other contract uses an internal transaction where the data is not in the transaction meta (e.g. when using the original Gnosis MultiSig contract)
    • It could be checked if encoding these values packed safes more gas

@ligi
Copy link

ligi commented Oct 15, 2019

Thanks for the fast PR to address #133 - event looks good to me - hope it will get merged.

@rmeissner
Copy link
Member Author

updated events for gas efficiency: transaction details have been removed from events, only the transaction hash and the payment are part of the event, the rest needs to be retrieved from other sources (e.g. the Ethereum transaction or our service).

Copy link
Member

@Uxio0 Uxio0 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@rmeissner rmeissner marked this pull request as ready for review October 28, 2019 23:14
@rmeissner rmeissner merged commit ef80fc7 into development Oct 28, 2019
@rmeissner rmeissner deleted the feature/issue_133 branch October 28, 2019 23:15
fdarian pushed a commit to fdarian/safe-contracts that referenced this pull request Jan 14, 2024
…vice-endpoint

Update Transaction Service endpoint in Safe Service Client tests
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.

Add events for receiving and sending
3 participants