|
| 1 | +# IGraphProxyAdmin Interface Signature Fix |
| 2 | + |
| 3 | +## Issue |
| 4 | + |
| 5 | +The IGraphProxyAdmin interface in `packages/interfaces/contracts/contracts/upgrades/IGraphProxyAdmin.sol` had incorrect function signatures that didn't match the actual GraphProxyAdmin contract implementation. |
| 6 | + |
| 7 | +### Understanding the Two Different `acceptProxy` Methods |
| 8 | + |
| 9 | +There are **two different contracts** with similar-sounding methods, which can cause confusion: |
| 10 | + |
| 11 | +1. **GraphUpgradeable** (base class for implementation contracts): |
| 12 | + |
| 13 | + ```solidity |
| 14 | + // Called ON the implementation contract |
| 15 | + function acceptProxy(IGraphProxy _proxy) external onlyProxyAdmin(_proxy) { |
| 16 | + _proxy.acceptUpgrade(); |
| 17 | + } |
| 18 | + ``` |
| 19 | + |
| 20 | + This is inherited by implementation contracts like RewardsManager, Staking, etc. |
| 21 | + |
| 22 | +2. **GraphProxyAdmin** (admin contract that manages upgrades): |
| 23 | + |
| 24 | + ```solidity |
| 25 | + // Called ON the admin contract, which then calls the implementation |
| 26 | + function acceptProxy(GraphUpgradeable _implementation, IGraphProxy _proxy) external onlyGovernor { |
| 27 | + _implementation.acceptProxy(_proxy); |
| 28 | + } |
| 29 | + ``` |
| 30 | + |
| 31 | + This is the admin contract that orchestrates upgrades. |
| 32 | + |
| 33 | +**IGraphProxyAdmin represents the second one** - the GraphProxyAdmin admin contract, not the GraphUpgradeable base class. |
| 34 | + |
| 35 | +### Incorrect Interface (Before) |
| 36 | + |
| 37 | +The interface mistakenly used the single-parameter signature from GraphUpgradeable: |
| 38 | + |
| 39 | +```solidity |
| 40 | +function acceptProxy(IGraphProxy proxy) external; |
| 41 | +
|
| 42 | +function acceptProxyAndCall(IGraphProxy proxy, bytes calldata data) external; |
| 43 | +``` |
| 44 | + |
| 45 | +### Actual GraphProxyAdmin Implementation |
| 46 | + |
| 47 | +From `packages/contracts/contracts/upgrades/GraphProxyAdmin.sol`: |
| 48 | + |
| 49 | +```solidity |
| 50 | +function acceptProxy(GraphUpgradeable _implementation, IGraphProxy _proxy) external onlyGovernor { |
| 51 | + _implementation.acceptProxy(_proxy); |
| 52 | +} |
| 53 | +
|
| 54 | +function acceptProxyAndCall( |
| 55 | + GraphUpgradeable _implementation, |
| 56 | + IGraphProxy _proxy, |
| 57 | + bytes calldata _data |
| 58 | +) external onlyGovernor { |
| 59 | + _implementation.acceptProxyAndCall(_proxy, _data); |
| 60 | +} |
| 61 | +``` |
| 62 | + |
| 63 | +The interface was **missing the first parameter** (`implementation` address) from both functions. It had copied the signature from GraphUpgradeable instead of using the correct GraphProxyAdmin signature. |
| 64 | + |
| 65 | +## Impact |
| 66 | + |
| 67 | +### Why This Mattered |
| 68 | + |
| 69 | +The deployment package (`@graphprotocol/deployment`) needs to call `acceptProxy` with the correct signature to upgrade proxy contracts. The function requires TWO parameters: |
| 70 | + |
| 71 | +1. The implementation contract address |
| 72 | +2. The proxy contract address |
| 73 | + |
| 74 | +Because the interface was wrong, the deployment code had to work around it by loading the full contract ABI instead of using the cleaner interface ABI: |
| 75 | + |
| 76 | +```typescript |
| 77 | +// packages/deployment/lib/abis.ts (old workaround) |
| 78 | +// Note: Load from actual contract, not interface, because IGraphProxyAdmin is outdated |
| 79 | +// Interface shows: acceptProxy(IGraphProxy proxy) |
| 80 | +// Contract has: acceptProxy(GraphUpgradeable _implementation, IGraphProxy _proxy) |
| 81 | +export const GRAPH_PROXY_ADMIN_ABI = loadAbi( |
| 82 | + '@graphprotocol/contracts/artifacts/contracts/upgrades/GraphProxyAdmin.sol/GraphProxyAdmin.json', |
| 83 | +) |
| 84 | +``` |
| 85 | + |
| 86 | +### Why Horizon is Not Affected |
| 87 | + |
| 88 | +GraphDirectory in horizon (`packages/horizon/contracts/utilities/GraphDirectory.sol`) imports and uses IGraphProxyAdmin, but **only as a type reference**: |
| 89 | + |
| 90 | +```solidity |
| 91 | +IGraphProxyAdmin private immutable GRAPH_PROXY_ADMIN; |
| 92 | +
|
| 93 | +constructor(address controller) { |
| 94 | + GRAPH_PROXY_ADMIN = IGraphProxyAdmin(_getContractFromController("GraphProxyAdmin")); |
| 95 | +} |
| 96 | +
|
| 97 | +function _graphProxyAdmin() internal view returns (IGraphProxyAdmin) { |
| 98 | + return GRAPH_PROXY_ADMIN; |
| 99 | +} |
| 100 | +``` |
| 101 | + |
| 102 | +GraphDirectory: |
| 103 | + |
| 104 | +- Stores the address as an immutable reference |
| 105 | +- Returns it via a getter function |
| 106 | +- **Never calls any methods on IGraphProxyAdmin** (like `acceptProxy`) |
| 107 | + |
| 108 | +Since horizon doesn't call the methods, fixing the interface signature doesn't break horizon. |
| 109 | + |
| 110 | +## Fix Applied |
| 111 | + |
| 112 | +### Updated Interface |
| 113 | + |
| 114 | +```solidity |
| 115 | +/** |
| 116 | + * @notice Accept ownership of a proxy contract |
| 117 | + * @param implementation The implementation contract accepting the proxy |
| 118 | + * @param proxy The proxy contract to accept |
| 119 | + */ |
| 120 | +function acceptProxy(address implementation, IGraphProxy proxy) external; |
| 121 | +
|
| 122 | +/** |
| 123 | + * @notice Accept ownership of a proxy contract and call a function |
| 124 | + * @param implementation The implementation contract accepting the proxy |
| 125 | + * @param proxy The proxy contract to accept |
| 126 | + * @param data The calldata to execute after accepting |
| 127 | + */ |
| 128 | +function acceptProxyAndCall(address implementation, IGraphProxy proxy, bytes calldata data) external; |
| 129 | +``` |
| 130 | + |
| 131 | +**Notes on parameter type choice:** |
| 132 | + |
| 133 | +- Used `address` instead of `GraphUpgradeable` for the implementation parameter |
| 134 | +- This avoids creating a dependency from interfaces package to contracts package |
| 135 | +- The actual contract uses `GraphUpgradeable`, but `address` is compatible (Solidity allows passing addresses for contract types) |
| 136 | +- The ABI encoding is identical - both produce the same function selector and parameter encoding |
| 137 | + |
| 138 | +**Call flow for context:** |
| 139 | + |
| 140 | +``` |
| 141 | +Deployer/Governor |
| 142 | + → GraphProxyAdmin.acceptProxy(implAddress, proxyAddress) ← IGraphProxyAdmin represents THIS |
| 143 | + → implAddress.acceptProxy(proxyAddress) ← GraphUpgradeable provides this |
| 144 | + → proxyAddress.acceptUpgrade() |
| 145 | +``` |
| 146 | + |
| 147 | +### Updated Deployment Code |
| 148 | + |
| 149 | +Removed the workaround comment and switched to using the interface: |
| 150 | + |
| 151 | +```typescript |
| 152 | +// packages/deployment/lib/abis.ts (now clean) |
| 153 | +export const GRAPH_PROXY_ADMIN_ABI = loadAbi( |
| 154 | + '@graphprotocol/interfaces/artifacts/contracts/contracts/upgrades/IGraphProxyAdmin.sol/IGraphProxyAdmin.json', |
| 155 | +) |
| 156 | +``` |
| 157 | + |
| 158 | +## Files Changed |
| 159 | + |
| 160 | +1. `packages/interfaces/contracts/contracts/upgrades/IGraphProxyAdmin.sol` |
| 161 | + - Fixed `acceptProxy` signature |
| 162 | + - Fixed `acceptProxyAndCall` signature |
| 163 | + |
| 164 | +2. `packages/deployment/lib/abis.ts` |
| 165 | + - Removed workaround comment |
| 166 | + - Changed to load from interface instead of full contract |
| 167 | + |
| 168 | +## Testing |
| 169 | + |
| 170 | +Build verification: |
| 171 | + |
| 172 | +- ✅ interfaces package builds successfully |
| 173 | +- ✅ deployment package dependencies build successfully |
| 174 | +- ✅ No TypeScript compilation errors |
| 175 | +- ✅ Hardhat compilation successful |
| 176 | + |
| 177 | +The deployment code in `packages/deployment/lib/upgrade-implementation.ts` already calls acceptProxy with both parameters: |
| 178 | + |
| 179 | +```typescript |
| 180 | +const acceptData = encodeFunctionData({ |
| 181 | + abi: GRAPH_PROXY_ADMIN_ABI, |
| 182 | + functionName: 'acceptProxy', |
| 183 | + args: [pendingImpl as `0x${string}`, proxyAddress as `0x${string}`], |
| 184 | +}) |
| 185 | +``` |
| 186 | + |
| 187 | +This call now works with the corrected interface ABI. |
| 188 | + |
| 189 | +## Recommendation |
| 190 | + |
| 191 | +This fix should be safe to merge. The interface now accurately reflects the actual contract implementation, and no existing code is broken by the change since: |
| 192 | + |
| 193 | +1. Deployment already expects the two-parameter signature |
| 194 | +2. Horizon only uses the type, never calls the methods |
| 195 | +3. The fix aligns the interface with reality, reducing confusion |
| 196 | + |
| 197 | +## Questions for Team Review |
| 198 | + |
| 199 | +1. Are there other consumers of IGraphProxyAdmin that might be affected? |
| 200 | +2. Should this be considered a breaking change requiring a major version bump of @graphprotocol/interfaces? |
| 201 | +3. Is there a reason the interface was historically wrong (legacy compatibility concerns)? |
0 commit comments