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

Make execTransactionIfApproved handling of failed transactions in team addition consistent with personal edition #36

Closed
Georgi87 opened this issue Jul 23, 2018 · 4 comments

Comments

@Georgi87
Copy link
Contributor

In the personal edition the nonce is increased even in case the transaction fails. We should have the same behavior for team edition: mark a transaction as executed even in case it fails and signal a failing transaction with an event. This prevents that transactions, which might not work at the time of transaction submission to remain forever in pending state and get lost, which is a potential security issue. Also execTransactionIfApproved should then return the success status of the transaction.

@rmeissner
Copy link
Member

I implemented this on https://github.com/gnosis/safe-contracts/tree/feature/improve_team_edition

I was asking myself if it would make more sense to provide a clearApproval. Mainly because if by any change the required gas changes and the transaction fails because of not enough gas, all confirmations have to be gathered again.

@rmeissner rmeissner self-assigned this Jul 23, 2018
@rmeissner rmeissner added this to In progress in Safe Contracts Progress Jul 27, 2018
@Georgi87
Copy link
Contributor Author

Georgi87 commented Aug 2, 2018

Who should be allowed to call clearApproval? This would require another multisig tx?

@rmeissner
Copy link
Member

@Georgi87 it would be more that each owner can clear his own approval (actually it shouldn't be required to be an owner to clear your approval, so that you can remove your approval even after you are not an owner anymore)

@rmeissner rmeissner added this to the Formal Verification milestone Aug 15, 2018
@rmeissner
Copy link
Member

Close because of #53

Safe Contracts Progress automation moved this from In progress to Done Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

2 participants