New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: optimistic asserter admin proposals #4397
Conversation
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
governorHub: RelayProposal[]; | ||
} | ||
|
||
const relayGovernanceRootTunnelMessage = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it feels like we now have a number of variants of this function spread over the scripts package (
const relayGovernanceRootTunnelMessage = async (targetAddress, message, governorRootTunnel) => { |
const relayGovernanceRootTunnelMessage = async ( |
what do you think we should do about this repetition but slight variation in implementation details?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in fact this function (and the following function) look to be exactly the same as that in 162 where we are dealing with ethers-based scripts. we could consider getting this PR in its current form, then, refactoring this src/upgrade-tests
to have an ethers
based sub-folder and then pull out these common functions into some common structure to remove this repetition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree @chrismaree,
I've done a refactor where I group all the relay functions in a common file
Also I've grouped the relays for the GovernorHub l2 chains (Arbitrum, Optimism, Boba) so they are relayed in a single transaction instead of 4: relayGovernanceHubMessages
If you think this covers your suggestions, after merging this PR we could refactor/delete 162 folder as register-new-contract as a generic way to register any new contract in mainnet + l2s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup! that refactor looks much better. we can move stuff around for the 162 folder once this is in. good call. I'm going to look more closely into the actual governance transactions produced here now and will report back.
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The relay transactions are difficult to test end to end but, with tenderly, I've managed to run separately the transactions once relayed to:
- Polygon (GovernorRootTunnel) I've tested the first and last function calls, add governor as a
CONTRACT_CREATOR
and add Finder implementation (the rest should logically work) by simulating a function call to:
contractAddress: 0xb4AeaD497FCbEAA3C37919032d42C29682f46376
inputData(this is only the first of 4 calls, it adds the Governor as a `CONTRACT_CREATOR`): 0x9a7c4b7100000000000000000000000000000000000000000000000000000000000000010000000000000000000000004f490f4835b3693a8874aee87d7cc242c25dccaf000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000c00000000000000000000000005f25b1647fa8eaea0e15edd413c7afcbe613b6f40000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000004474d0a6760000000000000000000000000000000000000000000000000000000000000001000000000000000000000000b4aead497fcbeaa3c37919032d42c29682f4637600000000000000000000000000000000000000000000000000000000
from: 0x8397259c983751daf40400790063935a11afa28a
contractAddress: 0xb4AeaD497FCbEAA3C37919032d42C29682f46376
inputData(this is the last function call, it adds the OA to the Finder): 0x9a7c4b7100000000000000000000000000000000000000000000000000000000000000000000000000000000000000004f490f4835b3693a8874aee87d7cc242c25dccaf000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000c000000000000000000000000009aea4b2242abc8bb4bb78d537a67a245a7bec640000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000004431f9665e4f7074696d6973746963417373657274657200000000000000000000000000000000000000000000000000001a3cf7c0f99256431fd6e8163ff8748a4ae50b6f00000000000000000000000000000000000000000000000000000000
from: 0x8397259c983751daf40400790063935a11afa28a
- Optimism (GovernorHub) I've tested the full registration (add Governor as creator, add OA to registry, remove Governor as creator, add OA to finder) by simulating the following function call:
contractAddress: 0xEF8b46765ae805537053C59f826C3aD61924Db45
inputData (encoded array of 4 function calls to add to Registry and Finder): 0x0cbfde46000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000003e00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000140000000000000000000000000000000000000000000000000000000000000022000000000000000000000000000000000000000000000000000000000000002e0000000000000000000000000a4199d73ae206d49c966cf16c58436851f87d47f0000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000004474d0a6760000000000000000000000000000000000000000000000000000000000000001000000000000000000000000ef8b46765ae805537053c59f826c3ad61924db4500000000000000000000000000000000000000000000000000000000000000000000000000000000a4199d73ae206d49c966cf16c58436851f87d47f0000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000006466c8c2500000000000000000000000000000000000000000000000000000000000000040000000000000000000000000cd5fe81278febf3a9323efc9f68aeccaeae8be2c000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a4199d73ae206d49c966cf16c58436851f87d47f000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000446be7658b0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000ef8b46765ae805537053c59f826c3ad61924db4500000000000000000000000000000000000000000000000000000000000000000000000000000000278d6b1aa37d09769e519f05fcc5923161a8536d0000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000004431f9665e4f7074696d697374696341737365727465720000000000000000000000000000000000000000000000000000cd5fe81278febf3a9323efc9f68aeccaeae8be2c00000000000000000000000000000000000000000000000000000000
from: 0x09afd24acc170c16f4ff64bdf2a4818c515440e8
// NODE_URL_10=<OPTIMISM-NODE-URL> \ | ||
// \ | ||
// NODE_URL_288=<OPTIMISM-NODE-URL> \ | ||
// \ | ||
// NODE_URL_137=<OPTIMISM-NODE-URL> \ | ||
// \ | ||
// NODE_URL_42161=<OPTIMISM-NODE-URL> \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these comments are wrong. only 10 is optimism.
const isPolygon = l2ChainId === 137; | ||
const isArbitrum = l2ChainId === 42161; | ||
|
||
const l2Registry = await getContractInstanceByUrl<Registry>("Registry", l2NodeUrl); | ||
|
||
// The l2Governor in polygon is the GovernorChildTunnel and in the rest of the l2's is the GovernorHub | ||
const l2Governor = await getContractInstanceByUrl<BaseContract>( | ||
isPolygon ? "GovernorChildTunnel" : "GovernorSpoke", | ||
l2NodeUrl | ||
); | ||
const l2Finder = await getContractInstanceByUrl<Finder>("Finder", l2NodeUrl); | ||
|
||
if (await l2Registry.isContractRegistered(l2NewContractAddress)) continue; | ||
|
||
console.log(`Registering ${l2NewContractAddress} on ${networkName}`); | ||
|
||
// Fund Arbitrum if needed for next 4 transactions | ||
if (isArbitrum) await fundArbitrumParentMessengerForRelays(arbitrumParentMessenger, proposerSigner, 5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kinda sucks that we need to do this for these two chains but it's not too bad. you could pre-fund the arbitrum setup to avoid needing to do this but I think it's better to show the full flow within the script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's required for this scripts to work, even when running them in mainnet, we still need to fund the arbitrumParentMessenger (it doesn't need to be during proposals though, but before the execution)
interface ProposedTransaction { | ||
to: string; | ||
data: string; | ||
value: string; | ||
} | ||
|
||
interface RelayTransaction { | ||
to: string; | ||
transaction: { | ||
name: string; | ||
params: { | ||
to: string; | ||
data?: string; | ||
chainId?: string; | ||
calls: { to: string; data: string }[]; | ||
}; | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these structured repeated elsewhere and can be re-used? they look like common cross-chain send transaction structures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved them to the common file as this types are custom
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
(await finder.getImplementationAddress(hre.ethers.utils.formatBytes32String(newContractName))).toLowerCase(), | ||
newContractAddressMainnet.toLowerCase() | ||
); | ||
console.log("Verified!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we dont actually verify that on the L2 things are done correctly, right? would we be able to run this against a hardhat fork of say Polygon to check that polygon has correctly added the contracts, via the cross-chain actions? i.e that the cross-chain addition worked correctly by re-running this on each chain? we might need to add case work for this (or a separate script). wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think you are right, I will add an extra script to this!
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
@@ -46,7 +46,7 @@ | |||
"eslint-plugin-react": "^7.19.0", | |||
"eslint-plugin-react-hooks": "^3.0.0", | |||
"ganache-cli": "^6.12.2", | |||
"hardhat": "^2.9.0", | |||
"hardhat": "^2.12.6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed to update hardhat to fix an issue when forking arbitrum documented here and fixed in the latest version of hardhat
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Updated: I've added a new script to simulate the relays calldata in every layer 2 chain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
import { | ||
ArbitrumParentMessenger, | ||
Finder, | ||
Governor, | ||
GovernorHub, | ||
GovernorRootTunnel, | ||
Proposer, | ||
Registry, | ||
} from "@uma/contracts-node/typechain/core/ethers"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these be imported directly from @uma/contracts-node? Note: if we're not happy with the naming (ethers vs web3), we can change it!
import { ArbitrumParentMessengerEthers } from "@uma/contracts-node";
I think that's the syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Fixed in latest commits
const hre = require("hardhat"); | ||
const assert = require("assert").strict; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these not be imported like the others? Does that break something? I'm especially surprised about assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
packages/scripts/src/utils/relay.ts
Outdated
relayedMessages.push( | ||
await relayGovernanceRootTunnelMessage(message.targetAddress, message.tx, l1Governor as GovernorRootTunnel) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just OOC, why did you choose to have the relayGovernanceRootTunnelMessage handle one message at once and push the results individually, whereas relayGovernanceHubMessages handles many messages at once?
Not a big deal, was just curious if there was some reason I missed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @mrice32, I decided to be consistent with how the GovernorRootTunnel.relayGovernance works natively vs the GovernorHub.relayGovernance. The latter has native support for multiple transactions in one whereas the GovernorRootTunnel's doesn't. I could have used a multicall contract in the destination chain I guess. Do you think it would be better or can you think of a better way to do it?
packages/scripts/src/utils/relay.ts
Outdated
|
||
if (apmBalance.lt(l1CallValue.mul(totalNumberOfTransactions))) { | ||
const amoutToSend = l1CallValue.mul(totalNumberOfTransactions).sub(apmBalance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think it's a bit easier to read if you put cost into a separate variable.
if (apmBalance.lt(l1CallValue.mul(totalNumberOfTransactions))) { | |
const amoutToSend = l1CallValue.mul(totalNumberOfTransactions).sub(apmBalance); | |
const cost = l1CallValue.mul(totalNumberOfTransactions); | |
if (apmBalance.lt(cost)) { | |
const amoutToSend = cost.sub(apmBalance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
import { BigNumberish } from "@ethersproject/bignumber"; | ||
import { BytesLike } from "@ethersproject/bytes"; | ||
import "@nomiclabs/hardhat-ethers"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import is required to fix typescript issues, without it ts
doesn't allow to do hre.ethers
import { interfaceName } from "@uma/common"; | ||
import { | ||
FinderEthers, | ||
GovernorEthers, | ||
GovernorHubEthers, | ||
GovernorRootTunnelEthers, | ||
ParentMessengerBaseEthers, | ||
ProposerEthers, | ||
RegistryEthers, | ||
} from "@uma/contracts-node"; | ||
import { BaseContract, PopulatedTransaction } from "ethers"; | ||
import hre from "hardhat"; | ||
import { getContractInstance, getContractInstanceByUrl } from "../../utils/contracts"; | ||
import { fundArbitrumParentMessengerForRelays, relayGovernanceMessages } from "../../utils/relay"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for keep recommending structural nits but one thing you could do that would make this a fair bit cleaner would be pull all the common/repeated imports from this chunk into a common.ts
file located in this directory then import them there and then just go import x from "./common";
. This should take the pretty repetitive code block of 30 or so lines repeated between the propose, verify and verify relays scripts down a lot.
packages/scripts/src/upgrade-tests/register-new-contract/3_VerifyRelays.ts
Outdated
Show resolved
Hide resolved
console.error(err); | ||
process.exit(1); | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is script is great! good work.
PROPOSAL_DATA=<PROPOSAL_DATA> yarn hardhat run ./src/upgrade-tests/register-new-contract/2_Verify.ts --network localhost | ||
``` | ||
|
||
2.3.2 Verify the result in layer 2 chains: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! good work on adding all these details and the extra verify cross-chain verification scripts.
…ifyRelays.ts Co-authored-by: Chris Maree <christopher.maree@gmail.com>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado pablo@umaproject.org
Motivation
Add scripts to propose the registration of the Optimistic Asserter in the Registry and Finder in mainnet and l2 chains (Polygon, Arbitrum, Optimism, Boba).
Summary
Testing
Check a box to describe how you tested these changes and list the steps for reviewers to test.
Issue(s)
Fixes #XXXX