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

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

Closed

Conversation

frontier159
Copy link

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
Copy link

github-actions bot commented May 7, 2023

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

@frontier159
Copy link
Author

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

@frontier159
Copy link
Author

@rmeissner and team - thoughts on this?

@mmv08
Copy link
Member

mmv08 commented May 13, 2023

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 🙏

@mmv08 mmv08 closed this May 13, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 13, 2023
@safe-global safe-global unlocked this conversation Jun 14, 2023
@akshay-ap akshay-ap added this to the Version 1.5.0 milestone Jun 14, 2023
@mmv08 mmv08 removed this from the Version 1.5.0 milestone Jul 10, 2023
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.

3 participants