search

safe-global / safe-smart-account

Safe allows secure management of blockchain assets.
https://safe.global
GNU Lesser General Public License v3.0
1.88k stars 927 forks source link

Add overloaded checkNSignatures to pass in the msg.sender #557

Closed frontier159 closed 1 year ago

frontier159 commented 1 year ago

PR raised for initial feedback/discussion. Tests can be added if the this is a sensible addition.

Motivation: When creating a guard (and perhaps module), there are use cases where signatures need to be checked vs some manual threshold. eg For a guard where I want to ensure very sensitive contracts/functions need a higher number of signers than the default safe threshold.

Utilising the exact checkNSignatures() logic of the safe is preferable to duplicating the logic within the guard.

As is, checkNSignatures() cannot be used, as msg.sender is used directly. So if called from the guard, the v == 1 check would fail for the pre-approved owner case.

Adding an overloaded checkNSignatures() version where the executor address is plumbed through will fix this elegantly.

I would love to collaborate on the guard I am developing further. Delegating the threshold calculation to a guard/module would be a killer built-in feature for Safe imo.

github-actions[bot] commented 1 year ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

frontier159 commented 1 year ago

I have read the CLA Document and I hereby sign the CLA

frontier159 commented 1 year ago

@rmeissner and team - thoughts on this?

mmv08 commented 1 year ago

After an internal discussion, we decided not to add this for two reasons:

  1. We do understand the issue here, but adding this adds an external trust dependency that the executor address is being passed correctly.
  2. We're not planning any more 1.x releases; in the v2 version, this functionality will be completely revised. You can read more about the v2 version here: https://forum.safe.global/t/safe-contract-v2/87

Thanks for the idea 🙏