feat: add support for UUPS proxy in QGB contract #177
Conversation
| @@ -35,7 +35,8 @@ contract RelayerTest is DSTest { | |||
|
|
|||
| validators.push(Validator(cheats.addr(testPriv1), votingPower)); | |||
| bytes32 hash = computeValidatorSetHash(validators); | |||
| bridge = new QuantumGravityBridge(initialVelsetNonce, (2 * votingPower) / 3, hash); | |||
| bridge = new QuantumGravityBridge(); | |||
| bridge.initialize(initialVelsetNonce, (2 * votingPower) / 3, hash); | |||
There was a problem hiding this comment.
unrelated to this change question
I don't think so, but is there ever a scenario where we have to be worried about this math?
There was a problem hiding this comment.
This is just a test, and we're defining ourselves the voting power for that validator. So it's okey
|
Turning to draft as I have some questions about the implementation I did, and I need to answer them before being 100% confident in this PR |
|
Reopening for review, the implementation looks fine |
src/QuantumGravityBridge.sol
Outdated
| import "openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initializable.sol"; | ||
| import "openzeppelin-contracts-upgradeable/contracts/proxy/utils/UUPSUpgradeable.sol"; | ||
| import "openzeppelin-contracts-upgradeable/contracts/access/OwnableUpgradeable.sol"; |
There was a problem hiding this comment.
Nit, but can these be ordered alphabetically (including path)?
src/QuantumGravityBridge.sol
Outdated
| @@ -31,7 +34,8 @@ struct Signature { | |||
| /// (see ./DataRootTuple.sol), with each tuple representing a single data root | |||
| /// in a Celestia block header. Relayed tuples are in the same order as the | |||
| /// block headers. | |||
| contract QuantumGravityBridge is IDAOracle { | |||
| /// @dev DO NOT REMOVE ANY INHERITED CONTRACT! They're essential for upgradability. | |||
There was a problem hiding this comment.
Since IDAOOracle isn't part of the upgradability logic, can it be removed later? If so then maybe document specifically which inherited contracts cant be removed.
Also, is there some way to enforce (potentially imperfectly, but at least as a first line of defence) that they aren't removed in CI? Maybe something like outputting a contract inheritance tree from the compiler and analyzing it.
There was a problem hiding this comment.
I've created the following bash script:
#!/usr/bin/env bash
out=$(surya inheritance src/QuantumGravityBridge.sol | grep -i "\"QuantumGravityBridge\" ->" | cut -d ">" -f 2 | sed 's/"//g' | sed 's/;//g')
required_contracts=("Initializable" "UUPSUpgradeable" "OwnableUpgradeable")
missing_contracts=()
for field in "${required_contracts[@]}"; do
if ! grep -q "\<$field\>" <<< "$out"; then
missing_contracts+=("$field")
fi
done
if [ ${#missing_contracts[@]} -eq 0 ]; then
echo "The QuantumGravityBridge contract is inheritting the right contracts. Exiting."
exit 0
else
echo "The QuantumGravityBridge contract is missing the necessary inherited contracts: ${missing_contracts[*]}"
exit 1
fi
I can add it in a separate PR.
Overview
Closes #146
This provides the UUPS upgradability mechanism for the QGB smart contract. The deployment and upgrade scripts will be handled in a separate issue.
Checklist