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

feat: Operator Access Module #52

Merged
merged 13 commits into from
Nov 18, 2024
Merged

Conversation

xorsal
Copy link
Contributor

@xorsal xorsal commented Nov 12, 2024

🤖 Linear

Closes GRT-227

Copy link

linear bot commented Nov 12, 2024

@xorsal xorsal changed the title feat: Operato Access Module feat: Operator Access Module Nov 12, 2024
src/interfaces/IEBOAccessModule.sol Outdated Show resolved Hide resolved
src/interfaces/IEBOAccessModule.sol Outdated Show resolved Hide resolved
src/contracts/EBOAccessModule.sol Outdated Show resolved Hide resolved
Comment on lines +19 to +20
/// @inheritdoc IEBOAccessModule
IHorizonStaking public horizonStaking;
Copy link
Member

Choose a reason for hiding this comment

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

HorizonAccountingExtension features an immutable state variable for HorizonStaking, which is inconsistent with how it is treated here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn’t aware of IHorizonStakingExtension::HORIZON_STAKING(). I wouldn't say it's inconsistent, it could definitely lead to unexpected behavior if the wrong addresses are set during deployment.

But more importantly, if horizonStaking can be obtained via a call (staticcall?) to horizonAccountingExtension, then I propose removing the horizonStaking state variable and instead calling HorizonStakingExtension::HORIZON_STAKING every time it’s needed, which is only inside of the hasAccess function.

Would this make sense to you?

Copy link
Member

Choose a reason for hiding this comment

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

Performing an external call ad infinitum to get an immutable variable doesn't make sense to me in terms of gas efficiency, compared to storing it once. The HorizonStaking address could be sent in the constructor or got through a single external call in it—however, this last option requires HorizonAccountingExtension to have been deployed, which might not be desirable.

Beware that HorizonStaking, HorizonStakingExtension and HorizonAccountingExtension refer to different contracts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beware that HorizonStaking, HorizonStakingExtension and HorizonAccountingExtension refer to different contracts.

My bad, I mixed up the contract names.

HorizonAccountingExtension is the one that has the external view HORIZON_STAKING

however, this last option requires HorizonAccountingExtension to have been deployed, which might not be desirable.

The HorizonAccountingExtension should be already deployed when EBOAccessModule is live because it's used in hasAccess to do the isAuthorized call.

And if that isn't the case, the module also have setHorizonAccountingExtension to update its address whenever necessary.

Copy link
Contributor Author

@xorsal xorsal Nov 18, 2024

Choose a reason for hiding this comment

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

now whenever the horizonAccountingExtension is updated, it will also call HORIZON_STAKING and update the horizonStaking address.

Relevant changes in: 630ce61

src/contracts/EBOAccessModule.sol Show resolved Hide resolved
@xorsal xorsal merged commit 03282d6 into feat/access-control Nov 18, 2024
7 checks passed
@xorsal xorsal deleted the feat/ebo-access-module branch November 18, 2024 16:10
xorsal added a commit that referenced this pull request Nov 18, 2024
Closes GRT-227

---------

Co-authored-by: Ashitaka <[email protected]>
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