dev: two step ownable

[milancermak]

Fixes #304

Adds support for two step ownership transfer, i.e. a new owner is first proposed by the current owner by calling transfer_ownership, then the proposed owner has to call accept_ownership to become new owner. Modeled after the Solidity version as linked in the original issue.

I kept the code in the ownable.cairo component because of code reuse. While I'm not a DRY fanatic, in this case I think it works pretty well. From a user's perspective, if they have a contract with (one step) Ownable and decide to upgrade to two step Ownable, they only have to switch from

 #[abi(embed_v0)]
 impl OwnableImpl = OwnableComponent::OwnableImpl<ContractState>;

to

#[abi(embed_v0)]
impl OwnableTwoStep = OwnableComponent::OwnableTwoStepImpl<ContractState>;

so it's a one line diff. Everything else (component! declaration and whatnot) stays the same and the contract upgrade is safe (current owner is not lost) as the underlying storage is the same.

The one slight drawback is that in the component impl, since both interfaces have functions of the same signature, I had to use the impl name to fully qualify which one should be run. In practice, it means going from self.transfer_ownership(newOwner) to Ownable::transfer_ownership(ref self, newOwner); in two or three places. It's a bit ugly IMO since it's inconsistent with the surrounding code, but because it's hidden inside the code of the component, I think it's a good trade-off. Curious to hear your thoughts.

PR Checklist

• Tests
• Tried the feature on a public network
• Documentation
• Added entry to CHANGELOG.md

[milancermak]

Seeking initial feedback on the proposed approach.

The whole test suite is missing, I'll add it once we flesh out the rest.

On more point I'd like to discuss - I always felt like there should be a way for the pending owner to renounce their nomination. This isn't present in the Solidity version (not sure if it was considered). I don't have a use case for it, just that it "feels fair" for the pending owner 😄 WDYT?

[martriay]

Thanks for pushing this. Overall direction looks really good to me! I appreciate the effort to streamline migration for existing Ownable users.

I kept the code in the ownable.cairo component because of code reuse.

I don't think it's just code reuse, I still believe it makes sense since this is to me just an alternative implementation of the same Ownable component. One drawback though is that this forces an extra storage element for vanilla Ownable users. It'd be useful to understand what's the cost difference (if any).

It's a bit ugly IMO since it's inconsistent with the surrounding code, but because it's hidden inside the code of the component, I think it's a good trade-off.

Agree on containing any ugliness within the component, away from the user interface. I presume using self results in a compilation error?

I always felt like there should be a way for the pending owner to renounce their nomination.

I honestly don't see the benefit, my bias is always towards minimizing complexity.

[milancermak]

One drawback though is that this forces an extra storage element for vanilla Ownable users. It'd be useful to understand what's the cost difference (if any).

My understanding is that there would only be a minimal increased cost when declaring a class, because of the increased bytecode size. I haven't verified it though. Otherwise, during runtime, there should be no difference whatsoever since the Ownable_pending_owner storage slot is not used (with the one step version).

I presume using self results in a compilation error?

Yes, something like error: Ambiguous method call. More than one applicable trait function with a suitable self type was found: IOwnable::transfer_ownership and IOwnableTwoStep::transfer_ownership. Consider adding type annotations or explicitly refer to the impl function.

my bias is always towards minimizing complexity.

That's a good point as the alternative would require another public function 👍

Will continue with adding tests and cleaning the PR up then.

[milancermak]

Code-wise the PR is ready for review (so I'm marking it as such).

However I haven't yet touched the docs section. How do you want to handle it? Should the two step process be a subsection under "Ownership and Ownable" or should it be its own section?

Should I test the component on testnet as well?

[milancermak]

Oh, forgot to mention - in the test file (test_ownable_twostep.cairo), I didn't test the internal functions that are already covered in test_ownable.cairo. Some tests are only copy-pasted (e.g. the whole section for renounce_ownership) since the functionality is the same in both versions. Finally, I added two more complex tests (test_full_two_step_transfer and test_pending_accept_after_owner_renounce), halfway between a unit test and a integration test 😅 Let me know if you want to keep or delete them.

[martriay]

However I haven't yet touched the docs section. How do you want to handle it? Should the two step process be a subsection under "Ownership and Ownable" or should it be its own section?

I'd add a new section, while documenting the component in the API section.

Should I test the component on testnet as well?

If you can do that, it'd be great. But to be fair we haven't been doing so since we're usually running behind the latest Cairo language which is not always deployed.

Thanks for the work put into this, and please bear with us while we make some space to properly review it.

[ericnordelo]

Looking very good @milancermak. Thanks for taking the time! I left some comments mostly related to consistency and comments, but I think is pretty much done.

[milancermak]

Dealt with all the points from the review. Thanks for the keen eye @ericnordelo.

Also did try the functionality on testnet. It works as designed. Even upgrading from one-step to two-step ownable (and back) is without any surprises.

Will proceed with writing the docs then.

[milancermak]

I forgot to do pendingOwner. Should I add it? On one hand, it's good for consistency, on the other, Camel is there for backwards compatibility and Cairo 1 is supposed to be snake_only and since two-step transfer wasn't there for Cairo 0, nothing breaks (I think). Thoughts?

[milancermak]

gm guys

Just pushed the docs - added both a section to the general overview and also updated the API docs.

Three things to point out:

1. I've copy-pasted the Usage section of TwoStepOwnable from Ownable, just changed the appropriate lines in code, is that alright?
2. Not quite sure what to put under the Interface section - note the TODO.
3. I've also added an "Upgrading from Ownable" subsection, as I think it might be helpful for users, but it renders poorly in the side menu 🙈

Thanks for any feedback 🚀

[martriay]

I forgot to do pendingOwner.

we already have acceptOwnership so we need to be consistent: we either have them both, or none. No strong opinion, either is fine

[ericnordelo]

Hey @milancermak , sorry for the delay. Thanks again for taking the time. I left some suggestions about doc organization.

[milancermak]

A valid point that two-step ownable is not a separate component. I've updated the docs based on the comments above, hopefully satisfactory. If not, feel free to unresolve the conversation or post new comments.

However I now feel that the docs don't show well enough how to use the two step functionality of the component. An easy copy-paste-able snippet like this would help IMO:

#[abi(embed_v0)]
impl OwnableTwoStepImpl = OwnableComponent::OwnableTwoStepImpl<ContractState>;

WDYT?

Also the mention about upgrading is now gone. Not sure how important that is.

[martriay]

Very nice work! I left some comments.

I'd like to see how the snippet looks in the docs, and I think we can forego the upgrading guide.

[martriay]

thanks for the heads up!

[martriay]

@milancermak is this ready for final review?

[milancermak]

Yes

[ericnordelo]

Looking good! Left some comments.

[milancermak]

GM sirs and happy new year. Just bumping this, I've addressed the latest comments. Let me know if there's anything else missing.

[milancermak]

Resurfacing this PR 😇

[martriay]

Thanks for the bump! This was already in my backlog for this week :)

[martriay]

Great progress, we're nearly there! I think we're ready to test this onchain, can you do it?
That and resolving the pending discussions should be enough.

[milancermak]

I already did the onchain testing some time ago - can't find the TXs anymore. But basically I did a super simple contract that was Ownable and Upgradable, did the upgrade back and forth, verified that the owner stayed the same during upgrades and that the new owner was transferred successfully after it was accepted and that a protected function could only be access by the owner. It worked smoothly.

[milancermak]

Should be ready for final review and merge @martriay @ericnordelo

[martriay]

Looks good to me! Let's address the merge conflicts.

[milancermak]

Done. Thanks @martriay

[martriay]

Thank you, for the contribution and the patience. Just requested @ericnordelo's approval (because he requested changes) to unblock the merge.

[ericnordelo]

LGTM! Thanks @milancermak for the time and patience. We need to refactor tests a bit for consistency with our last updates, but we don't need to stale the PR because of that IMO. That can be fixed later.