Add blockbuilderpolicy upgradeability #15
Conversation
* add tests for upgradeability and fix tests that are obsolete because of the new upgradeability changes
Melvillian
force-pushed
the
add-blockbuilderpolicy-upgradeability
branch
2 times, most recently
from
f9b977f to
cb566ef
Compare
| @@ -4,12 +4,6 @@ | |||
| [submodule "lib/automata-dcap-attestation"] | |||
| path = lib/automata-dcap-attestation | |||
| url = https://github.com/automata-network/automata-dcap-attestation | |||
| [submodule "lib/solmate"] | |||
There was a problem hiding this comment.
I removed these because we use OZ's OwnableUpgradeable contract instead of solmate's Ownable, and we don't need the top level openzeppelin-contracts dep anymore because we use remappings.txt to point to the openzeppelin-contracts/ dep of openzeppelin-contracts-upgradeable (see remappings.txt)
Melvillian
force-pushed
the
add-blockbuilderpolicy-upgradeability
branch
from
cb566ef to
2e6d338
Compare
Melvillian
force-pushed
the
add-blockbuilderpolicy-upgradeability
branch
from
b12988b to
b0862e4
Compare
… RegisterTeeScript
| function setUp() public {} | ||
|
|
||
| function run() public { | ||
| vm.startBroadcast(); | ||
| policy = new BlockBuilderPolicy( | ||
| vm.envAddress("FLASHTESTATION_REGISTRY_ADDRESS"), vm.envAddress("OWNER_BLOCK_BUILDER_POLICY") | ||
| // this is the address that stores all of the TEE-controlled addresses and their associated workloadIds |
There was a problem hiding this comment.
For any production deploys, would recommend just adding this to the contracts deploy cli tool, unless its going to be like a one time deployment (ie only on 1 chain)
There was a problem hiding this comment.
sorry, what do you mean "contracts deploy cli tool"? What is that? And once I know what it is, why would we add it to this cli tool, what are we trying to improve?
There was a problem hiding this comment.
Its just a nice way to deploy contracts if we have to deploy many of the same thing, if we don't need to do that here then its prob not worth it
There was a problem hiding this comment.
OK I see what you mean.
Deployment has been pretty easy so far. I would add this if we get more orgs across multiple chains trying to deploy. But until that happens I'd rather keep it as it is (more simple)
| require(registry != address(0), "FLASHTESTATION_REGISTRY_ADDRESS address is 0x0"); | ||
| FlashtestationRegistry registryContract = FlashtestationRegistry(registry); | ||
| require( | ||
| registryContract.owner() == vm.envAddress("FLASHTESTATION_REGISTRY_OWNER"), |
There was a problem hiding this comment.
it seems like the registry contract is already deployed at this point? why do we need to asset the registry owner here?
There was a problem hiding this comment.
it's just a sanity check to make sure that, before we go ahead and make the script's function call, we ensure we're working with the registry contract that we think we're working with.
No description provided.