Skip to content

Add Ownable2StepSign, an extension of Ownable2Step #5628

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

markolazic01
Copy link

@markolazic01 markolazic01 commented Apr 5, 2025

This PR contains an extension of Ownable2Step, that I found useful for the following reasons:

  • Reduces the amount of needed transactions for execution of an ownership transfer from 2 to 1 (for EOAs), while keeping the same level of protection as present in Ownable2Step.
  • Provides an additional level of safety to the newOwner wallet at the time of the ownership transfer, as it can stay offline and therefore reduce the potential security threats.

Ownership over the newOwner EOA, as well as the acceptance of ownership transfer are proved via signature.
Call for an ownership transfer must be made by the current owner (as per usual), with the signature provided by the newOwner.

Support for 2step ownership transfer to contracts is inherited from Ownable2Step.

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

Copy link

changeset-bot bot commented Apr 5, 2025

🦋 Changeset detected

Latest commit: cc3b139

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@markolazic01 markolazic01 changed the title Add Ownable2StepSign Add Ownable2StepSign, an extension of Ownable2Step Apr 6, 2025
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

Overall, I don't think there is anything terribly wrong with this PR in terms of design ... But I'm also not sure we want it.

That is not something we got many requests for, and its something that may further reduce the discoverability of our many features.

Maybe we want to have that in the community repository ?

@ernestognw, @arr00, @gonzaotc WDYT ?

@Amxx
Copy link
Collaborator

Amxx commented Apr 7, 2025

Note:

  • I would move that to the community repo instead.
  • This contract shouldn't be names "two steps" because its all in one operation. Instead it could be something like OwnableRecipientSignOff (to be discussed)
  • Right now the contract assumes that its called by the owner, with the recipient signature. Should we also consider the case where the owner provides a signature, and the recipient calls? Or the case where both side provide a signature and anyone does the call?

@markolazic01
Copy link
Author

Note:

  • I would move that to the community repo instead.
  • This contract shouldn't be names "two steps" because its all in one operation. Instead it could be something like OwnableRecipientSignOff (to be discussed)
  • Right now the contract assumes that its called by the owner, with the recipient signature. Should we also consider the case where the owner provides a signature, and the recipient calls? Or the case where both side provide a signature and anyone does the call?

With the coming changes, especially referring to the inheritance of Ownable2Step being switched to Ownable, I will update the name (at least temporarily) to OwnableRecipientSignOff.

When it comes to the cases mentioned in the last point @Amxx, I have thought about that as well. My current conclusion is that existing form is the safest of the mentioned three options. This would be mostly due to the social aspects, as I feel that users are much more prone to giving away signatures then they are to making suspicious transactions. Also, I feel like it is safest to keep everything in the hands of the current owner, even when the owner receives the 'newOwner' signature, it will be possible for him to rethink the ownership transfer if needed (and if 'owner' is the one providing the signature, there will be no possibility to rethink).
This is, of course, just my view on this. I'd be happy to discuss the other aspects.

*/
error InvalidSigner();

constructor(string memory name) EIP712(name, "1") {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
constructor(string memory name) EIP712(name, "1") {}

Copy link
Author

Choose a reason for hiding this comment

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

I've added this only to fix the version to 1, if that is not desirable I will remove the constructor.

Copy link
Author

Choose a reason for hiding this comment

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

Hey @Amxx, just pinging to make sure you want this suggestion applied (considering the comment that I previously made).

@markolazic01
Copy link
Author

Hey guys @Amxx @ernestognw @arr00 @gonzaotc, sorry to bother you, any chance we can proceed with this?

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.

2 participants