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

Classic Ownable(or Owned) or Ownable2Step #15

Closed
MerlinEgalite opened this issue Jul 3, 2023 · 14 comments · Fixed by #43
Closed

Classic Ownable(or Owned) or Ownable2Step #15

MerlinEgalite opened this issue Jul 3, 2023 · 14 comments · Fixed by #43
Assignees
Labels

Comments

@MerlinEgalite
Copy link
Contributor

No description provided.

@MerlinEgalite MerlinEgalite mentioned this issue Jul 5, 2023
@MerlinEgalite MerlinEgalite linked a pull request Jul 5, 2023 that will close this issue
@Rubilmax
Copy link
Collaborator

Rubilmax commented Jul 6, 2023

Related discussion: #43 (comment)

@MerlinEgalite
Copy link
Contributor Author

Original discussion: #43 (comment)

The issue was raised before the discussion but that does not matter

@MathisGD
Copy link
Contributor

MathisGD commented Jul 6, 2023

I'm not even sure that we need a transferOwnership function. So I would say no (transferOwnership will never be used).

@QGarchery
Copy link
Contributor

QGarchery commented Jul 6, 2023

The one use case I can see for transferring ownership is when deciding to put the fee back to 0 forever, as a donation of the protocol to the world 🤩 (so only renounceOwnership)

@MerlinEgalite
Copy link
Contributor Author

There might be a governance setup update that is necessary to do for whatever reason we would be stuck with the old system.

@MathisGD
Copy link
Contributor

MathisGD commented Jul 6, 2023

Can't the DAO deploy blue ?

@Rubilmax
Copy link
Collaborator

Rubilmax commented Jul 7, 2023

Can't the DAO deploy blue ?

I believe there can be a sudden case where the DAO absolutely needs to transfer ownership. I don't think saving a single function is worth it here.

@MerlinEgalite
Copy link
Contributor Author

After discussion we've decided to keep it, as we might need to change to a cleaner governance setup later on

@MathisGD
Copy link
Contributor

MathisGD commented Jul 8, 2023

I still think that we don't need 2 steps.

@Rubilmax
Copy link
Collaborator

Rubilmax commented Jul 8, 2023

I still think that we don't need 2 steps.

The reason I suggested 2 steps first is that Spearbiit suggested it in ma3, to avoid transferring to a dead address. I honestly don't think we need any kind of input sanitization, so I'm ok to not have 2 steps. I'm still in favor of inheriting OZ (rather than rebuilding it ourselves).

@MerlinEgalite
Copy link
Contributor Author

Ok since we've decided to implement events (#36) the only blocker for not using an already written library is code philosophy, am I right?

@Rubilmax
Copy link
Collaborator

Everything can be set subjective lol but it can also be about safety: renounceOwnership vs transferOwnership(address(0)). Let's go for OZ's Ownable?

@MerlinEgalite
Copy link
Contributor Author

There's another blocker sorry: error management #30. OZ is using custom errors for instance.

At the very beginning of my PR #43 I used Owned from solmate (using require).

I don't have a strong opinion on that.

@Rubilmax Rubilmax self-assigned this Jul 11, 2023
@Rubilmax Rubilmax linked a pull request Jul 11, 2023 that will close this issue
@Rubilmax Rubilmax removed a link to a pull request Jul 24, 2023
@MathisGD
Copy link
Contributor

Settled: we are going with our own implementation of a simple Owned, for simplicity and consistency of events and errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants