cairo-contracts
cairo-contracts copied to clipboard
OpenZeppelin
Feature/transfer-ownership-pattern-#275
Resolves #304. This PR implements code in the Ownable contract to introduce a two step transfer ownership process plus zero address checks.
This is optional as the original pattern is still present, however now contract owners can propose a new owner rather than directly transfer ownership. The new owner is required to accept the request before ownership is transferred. Both the owner and the proposed owner can cancel the request.
The PR contains:
- updated ownable.sol contract with zero address checks added too.
- updated mock Ownable contract in the test directory.
Het @ctrlc03 thank you for submitting this PR. This looks really cool! We've been deliberating on using namespace in the contracts and reconsidering new conventions for function naming (see the discussion in #267). Unfortunately, reviewing PRs has been blocked because of this, but we'll take a look at this as soon as we can!
Hey @ctrlc03, just checking in. We updated the Ownable library, so it's no longer a blocker. No pressure, but did you want to continue working on this? Let us know either way :)
Hey @ctrlc03, just checking in. We updated the Ownable library, so it's no longer a blocker. No pressure, but did you want to continue working on this? Let us know either way :)
Hey @andrew-fleming - awesome, I will then be updating it in the coming days. Was still running with issues when running the tests with Tox but will try to sort it out.
Hey, @ctrlc03! Just checking in. Were you able to make any headway on this PR? Again, no pressure! Let us know if you're too busy. We can take over and finish this up
Hey, @ctrlc03! Just checking in. Were you able to make any headway on this PR? Again, no pressure! Let us know if you're too busy. We can take over and finish this up
My apologies, been crazy busy lately. Finishing up now - writing up the tests hoping that they finally work on my setup :)
My apologies, been crazy busy lately. Finishing up now - writing up the tests hoping that they finally work on my setup :)
No worries! And that's excellent to hear!
My apologies, been crazy busy lately. Finishing up now - writing up the tests hoping that they finally work on my setup :)
No worries! And that's excellent to hear!
Code and tests are ready and working! I've replied by email to your previous comment, unsure you saw that. Basically asking (as a git noob) what's the best way to push the changes considering that this PR runs on an outdated version of the contracts. I have made the changes on a separate branch, just not sure if I should be pushing a new PR, or overwrite here (that might mess up something?).
Hmm I unfortunately did not receive said email, but I'll answer here. First, make sure your forked repo is up to date by going to fetch upstream and click fetch and merge. Then, in your local branch, write in the command line: git pull origin main. It's good practice if you want to go through the conflicts and resolve them. There's other tricks with git, but this approach is simple and easy. If, however, there's a ridiculous amount of conflicts (rebasing hell), it might be easier to just close the PR and create a new one
Good luck, let me know how it goes!
I somehow made it! lost the past commits but I think that's fine. The code works and so do tests. I've added a couple new events and one check on the cancelOwnershipRequest -> basically it checks that the proposed owner is not already zero, to prevent re-setting it to zero. Let me know if that's ok.
Sorry for coming in late to the discussion. I will say that I don't mind if this functionality is added by default in Ownable. It gives the user of Ownable a choice to use either one-step or two-step ownership transfer depending on their situation. I'm considering that this is how we should implement the feature in the Solidity contracts, though haven't discussed it with the team yet.
...I will say that I don't mind if this functionality is added by default in Ownable. It gives the user of Ownable a choice to use either one-step or two-step ownership transfer depending on their situation. I'm considering that this is how we should implement the feature in the Solidity contracts, though haven't discussed it with the team yet.
Thanks for your input^ My concern with adding the functionality by default is with standardizing the interface. I think it'd make the distinction easier for users if we treated the two-step pattern as an Ownable extension (similar to ERC721Enumerable's relationship to ERC721). Since this feature is forthcoming in OZ's Solidity implementation, we'll follow suit either way.
@ctrlc03 😅 I'm really sorry! I think it's best to wait on the Solidity implementation and use their design as a template for interoperability. I know you put a lot of hard work in on this and I very much appreciate the effort 🙏
...I will say that I don't mind if this functionality is added by default in Ownable. It gives the user of Ownable a choice to use either one-step or two-step ownership transfer depending on their situation. I'm considering that this is how we should implement the feature in the Solidity contracts, though haven't discussed it with the team yet.
Thanks for your input^ My concern with adding the functionality by default is with standardizing the interface. I think it'd make the distinction easier for users if we treated the two-step pattern as an Ownable extension (similar to ERC721Enumerable's relationship to ERC721). Since this feature is forthcoming in OZ's Solidity implementation, we'll follow suit either way.
@ctrlc03 😅 I'm really sorry! I think it's best to wait on the Solidity implementation and use their design as a template for interoperability. I know you put a lot of hard work in on this and I very much appreciate the effort 🙏
No worries! Let's keep this on standby for now :) thanks for your help so far with this!
No worries! Let's keep this on standby for now :) thanks for your help so far with this!
My pleasure, and thank you!
Hi Andrew, Git noob here, I have made all changes locally in a separate branch as I couldn’t pull from main, however, what’s the best way to submit the new changes considering all other files changed too in the original repo?
Should we close the PR and re submit or is there another way?
Thanks
On Thu, 2 Jun 2022 at 03:42, Andrew Fleming @.***> wrote:
Hey, @ctrlc03 https://github.com/ctrlc03! Just checking in. Were you able to make any headway on this PR? Again, no pressure! Let us know if you're too busy. We can take over and finish this up
— Reply to this email directly, view it on GitHub https://github.com/OpenZeppelin/cairo-contracts/pull/275#issuecomment-1144358362, or unsubscribe https://github.com/notifications/unsubscribe-auth/AWI6QCWAUOM3VFOQQWH3WG3VNANPRANCNFSM5TGYY7HQ . You are receiving this because you were mentioned.Message ID: @.***>
Hey, @ctrlc03!
So when there's conflicts in a PR, it's a good idea to checkout that branch, git merge, and fix the conflicts. It might make sense to just make a new PR since this PR is so old.
And that's great that you have changes for this issue! We have Ownable2Step to follow now as a blueprint :)
Hey, @ctrlc03!
So when there's conflicts in a PR, it's a good idea to checkout that branch,
git merge, and fix the conflicts. It might make sense to just make a new PR since this PR is so old.And that's great that you have changes for this issue! We have Ownable2Step to follow now as a blueprint :)
Hey! yup good shout, I'll work on a new PR in the next days :)