Skip to content

Add Guardian contract#44

Merged
QuentinI merged 13 commits intomainfrom
ag/guardians
Jan 29, 2026
Merged

Add Guardian contract#44
QuentinI merged 13 commits intomainfrom
ag/guardians

Conversation

@QuentinI
Copy link
Copy Markdown
Contributor

@QuentinI QuentinI commented Jan 26, 2026 •
edited
Loading

Closes #<ISSUE_NUMBER>

This PR:

  • Adds an OwnableWithGuardiansUpgradeable abstract contract that mostly inherits from OpenZeppelin's Ownable2Step, AccessControlEnumerable and UUPSUpgradeable, only adding some convenience functions to predefine the guardian role
  • Modifies existing contracts to inherit from it
  • Makes setEnclaveHash() and deleteEnclaveHashes() callable by guardians
  • Updates deployment scripts to use proxy deployment

This PR does not:

  • Change any verification logic
  • Set limit on guardian number

Key places to review:

src/OwnableWithGuardiansUpgradeable.sol for the new contract itself
src/TEEHelper.sol and src/EspressoTEEVerifier.sol for usage of it

@QuentinI QuentinI mentioned this pull request Jan 26, 2026
Closed
Sneh1999
Sneh1999 reviewed Jan 26, 2026
View reviewed changes
Sneh1999
Sneh1999 reviewed Jan 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@Sneh1999 Sneh1999 left a comment

Choose a reason for hiding this comment

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

@QuentinI I have a general PR to make tee contracts upgradable, I think you should maybe build your code on top of that?

shenkeyao
shenkeyao reviewed Jan 28, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@shenkeyao shenkeyao left a comment

Choose a reason for hiding this comment

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

A general question: why do we inherit OpenZeppelin's UUPSUpgradeable? For OP, we decided to follow the transparent proxy pattern since that's what Celo does. I guess we can use UUPS here because this repo isn't for OP only, but is this strictly needed?

@Sneh1999
Copy link
Copy Markdown
Contributor

Sneh1999 commented Jan 28, 2026

A general question: why do we inherit OpenZeppelin's UUPSUpgradeable? For OP, we decided to follow the transparent proxy pattern since that's what Celo does. I guess we can use UUPS here because this repo isn't for OP only, but is this strictly needed?

yea I think we should use transparent proxy here as well, I have used that in my PR

@QuentinI QuentinI force-pushed the ag/guardians branch 3 times, most recently from 9bec124 to e312610 Compare January 28, 2026 22:56
@QuentinI
Upgrade OZ contracts
531640a
@QuentinI
Add guardian contract
209d3a2
@QuentinI
Use guardian contract
0eb827e
@QuentinI
Add OwnableWithGuardiansUpgradeable and convert contracts to upgradeable
0017b4b 
- Add OwnableWithGuardiansUpgradeable combining Ownable2Step, AccessControlEnumerable, and UUPS
- Define GUARDIAN_ROLE with onlyOwner, onlyGuardian, and onlyGuardianOrOwner modifiers
- Convert EspressoTEEVerifier, EspressoSGXTEEVerifier, EspressoNitroTEEVerifier to UUPS upgradeable
- Update TEEHelper base contract to use OwnableWithGuardiansUpgradeable
- Update setEnclaveHash() and deleteEnclaveHashes() to onlyGuardianOrOwner
- Update all deployment scripts to use ERC1967Proxy pattern
- Add optional initial enclave hash setting in deployment scripts
- Update all tests for proxy deployment and guardian error types
- Add comprehensive test coverage (39 tests for OwnableWithGuardiansUpgradeable)
@QuentinI
Formatting
b76a5e4
@QuentinI
Use TransparentUpgradeableProxy
c8121eb
@QuentinI
Fix rebase
517dbaa
@QuentinI
Fix rebase x2
2d0e602
@QuentinI
Formatting
8df3377
Sneh1999
Sneh1999 reviewed Jan 29, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@Sneh1999 Sneh1999 left a comment

Choose a reason for hiding this comment

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

@QuentinI Why are we updating openzeppelin contract and upgradable contracts lib?

Sneh1999
Sneh1999 reviewed Jan 29, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@Sneh1999 Sneh1999 left a comment

Choose a reason for hiding this comment

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

Can you also update all tests which were currently testing only owner can update the values to also guardian can update the values? 🙏

Sneh1999
Sneh1999 reviewed Jan 29, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@Sneh1999 Sneh1999 left a comment

Choose a reason for hiding this comment

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

we might also have toy update the deployment and tests scripts to specify a guradian address when deploying the Espresso tee contract 🤔

@QuentinI
Copy link
Copy Markdown
Contributor Author

QuentinI commented Jan 29, 2026

@Sneh1999 I ran into issues with OZ and OZ-upgradeable being of incompatible versions, I don't remember the specific error.
I can try to downgrade and resolve the issue in another way if we don't want them updated.

Addressed other comments.

Sneh1999
Sneh1999 approved these changes Jan 29, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@Sneh1999 Sneh1999 left a comment

Choose a reason for hiding this comment

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

LGTM!

@QuentinI QuentinI merged commit cb1494e into main Jan 29, 2026
3 checks passed
@QuentinI QuentinI mentioned this pull request Jan 30, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shenkeyao shenkeyao shenkeyao left review comments

@Sneh1999 Sneh1999 Sneh1999 approved these changes

@ImJeremyHe ImJeremyHe Awaiting requested review from ImJeremyHe ImJeremyHe is a code owner

@alysiahuggins alysiahuggins Awaiting requested review from alysiahuggins alysiahuggins is a code owner

@zacshowa zacshowa Awaiting requested review from zacshowa zacshowa is a code owner

@jbearer jbearer Awaiting requested review from jbearer jbearer is a code owner

@lukeiannucci lukeiannucci Awaiting requested review from lukeiannucci lukeiannucci is a code owner

@jjeangal jjeangal Awaiting requested review from jjeangal jjeangal is a code owner

@Ayiga Ayiga Awaiting requested review from Ayiga Ayiga is a code owner

@philippecamacho philippecamacho Awaiting requested review from philippecamacho philippecamacho is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@QuentinI @Sneh1999 @shenkeyao