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

Remove Ownable dependency in AxelarGatewayBase #64

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

Conversation

ernestognw
Copy link
Member

@ernestognw ernestognw commented Jan 9, 2025

Motivation

This allow to setup custom access control easily (e.g. through Wizard)

@ernestognw ernestognw requested a review from frangio January 9, 2025 00:25
@ernestognw ernestognw marked this pull request as ready for review January 9, 2025 00:44
@ernestognw ernestognw requested review from cjcobb23 and a team as code owners January 9, 2025 00:44
@Amxx
Copy link
Collaborator

Amxx commented Jan 9, 2025

Was that requested ?

In general, I can see the point of having flexibility in the access control mechanism ... but I'm not sure if this contract really needs it. You mention this could be setup through the wizard, but the wizard is for contracts that app developpers will deploy, and for which we need many instances with different characteristics. In this case I only expect one instance per chain to exist. IMO it should be deployed, and owned, by axelar. The ownership would be an axelar multisig, or governor, and that is it.

@ernestognw
Copy link
Member Author

Not that it was requested but it's just too strict. Even if it's owned by Axelar it's too restrictive that we're only allowing Ownable.,

Also note that the gateways are adapters, it'd make sense if it was the canonical gateway. I think this change is not really controversial and just allows for better flexibility, which is what we want for a library in the end

@@ -31,6 +30,12 @@ abstract contract AxelarGatewayBase is Ownable {
mapping(string caip2 => string remoteGateway) private _remoteGateways;
mapping(string caip2OrAxelar => string axelarOrCaip2) private _chainEquivalence;

/// @dev Only allows an authorized registrant to call the function. See {_checkRegistrant}.
modifier onlyRegistrant() {
_checkRegistrant();
Copy link
Contributor

Choose a reason for hiding this comment

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

This pattern usually goes like this:

Suggested change
_checkRegistrant();
_checkRegistrant(msg.sender);

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