feat: Add transfer admin role workflow and scripts for multi-chain su… #90
Conversation
gfournierPro
requested review from
Le-Caignec,
Copilot and
zguesmi
and removed request for
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive admin role transfer functionality for multi-chain smart contract management. It implements a two-phase admin transfer process with initiation and acceptance steps for secure role transitions.
- Adds
TransferAdminRole.s.solscript with contracts for initiating and accepting admin role transfers - Updates Makefile with commands for single-chain admin role operations
- Introduces GitHub Actions workflow for automated admin role transfers via UI
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| script/TransferAdminRole.s.sol | Core script implementing two-phase admin transfer for RLCLiquidityUnifier, RLCCrosschainToken, and IexecLayerZeroBridge contracts |
| Makefile | Adds make targets for transferring and accepting admin roles on single chains |
| .github/workflows/transfer-admin.yml | GitHub Actions workflow for triggering admin transfers through web interface |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #90 +/- ##
=======================================
Coverage ? 83.78%
=======================================
Files ? 4
Lines ? 111
Branches ? 7
=======================================
Hits ? 93
Misses ? 17
Partials ? 1 ☔ View full report in Codecov by Sentry. |
| - ethereum | ||
| - arbitrum | ||
| - sepolia | ||
| - arbitrum_sepolia |
There was a problem hiding this comment.
use mainnets & testnets here
| required: true | ||
| type: string | ||
|
|
||
| jobs: |
There was a problem hiding this comment.
use matrix here to made that on multiple network as it was made for configuration script
There was a problem hiding this comment.
are we sure we can transmit the ownership to the same wallet (multi-sig) address other all networks ? @zguesmi
There was a problem hiding this comment.
Yes it's better to do each chain separately to avoid issues. For example if we use a multisig that is not available on a specific network.
There was a problem hiding this comment.
it could be nice to move run() function as the first function of each contracts
There was a problem hiding this comment.
|
We need to test these scripts, for example, to make sure that we always transfer ownership of all relevant contracts at once. |
…contract transfers
…racts and introducing helper functions
|
What do you think of this? |
| function run() external pure override(BeginTransferAdminRole, AcceptAdminRole) { | ||
| // This function should not be called directly in tests | ||
| // Use super.run_beginTransfer() or super.run_acceptAdmin() instead | ||
| // Use super.beginTransferForAllContracts() or super.run_acceptAdmin() instead |
There was a problem hiding this comment.
| // Use super.beginTransferForAllContracts() or super.run_acceptAdmin() instead | |
| // Use super.beginTransferForAllContracts() or super.acceptAdminRoleTransfer() instead |
test/units/utils/TestUtils.sol
Outdated
| fee = layerZeroContract.quoteSend(sendParam, false); | ||
| } | ||
|
|
||
| function emptyConfigParams() internal pure returns (ConfigLib.CommonConfigParams memory) { |
There was a problem hiding this comment.
This is not needed, declaring ConfigLib.CommonConfigParams memory params; is enough to create a default empty object.
| * @notice External wrapper for beginTransfer to enable proper revert testing | ||
| * @dev This function is necessary for testing revert scenarios because: | ||
| * When testing internal functions that call `super`, | ||
| * Foundry's vm.expectRevert doesn't properly catch reverts from internal function calls. | ||
| * This is because internal calls don't create new call frames that Foundry can intercept. | ||
| * | ||
| * This pattern allows vm.expectRevert to work correctly by ensuring the revert happens | ||
| * in a separate call frame that Foundry can detect and validate. |
There was a problem hiding this comment.
Turning the function beginTransfer public allows to make external calls.
Test on anvil sepolia fork after a re-deployment
ex : old admin
0xf91Cc8E418A71b1598082EB3D20F4d5DaDeEaa74new admin0x70997970C51812dc3A010C7d01b50e0d17dc79C8than :