-
Notifications
You must be signed in to change notification settings - Fork 308
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: proposer forwarder contract #11403
base: master
Are you sure you want to change the base?
feat: proposer forwarder contract #11403
Conversation
c3f76cd
to
339bc31
Compare
23256d2
to
f72faad
Compare
yarn-project/end-to-end/src/e2e_p2p/upgrade_governance_proposer.test.ts
Outdated
Show resolved
Hide resolved
@@ -190,7 +200,13 @@ export interface DeployL1ContractsArgs extends L1ContractsConfig { | |||
|
|||
export type L1Clients = { | |||
publicClient: PublicClient<HttpTransport, Chain>; | |||
walletClient: WalletClient<HttpTransport, Chain, Account>; | |||
walletClient: Client< |
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.
curious as to why this had to change from the WalletClient type
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 is to merge in the public actions with the wallet client.
}); | ||
|
||
if (rollupFunctionName === 'propose') { | ||
rollupData = callData; |
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 feel like we can just return immediately here. and skip reducing the rollup calls
@@ -60,6 +63,23 @@ export class AnvilTestWatcher { | |||
await this.filledRunningPromise?.stop(); | |||
} | |||
|
|||
async mineIfOutdated() { |
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 must have been a pain
const forwarderContract = | ||
config.customForwarderContractAddress && config.customForwarderContractAddress !== EthAddress.ZERO | ||
? new ForwarderContract(publicClient, config.customForwarderContractAddress.toString()) | ||
: await ForwarderContract.create(walletClient.account.address, walletClient, publicClient, log); |
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.
when moving to live networks we should probably add a command to our CLI that does this initialisation, we dont want to have people accidentally keep deploying these over config errors
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 call. I filed 11603
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.
minor comments, huge effort well done
ccb3aa2
to
a367bd3
Compare
00da5ff
to
b52ffd1
Compare
}; | ||
} | ||
|
||
public async tryGetErrorFromRevertedTx( |
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.
some of this is covered in formatViemError function.
It's a bit of a mess and I wanted to spend some time to get it actually right, wonder if it'd be worth me doing on top of this PR (as a separate one)
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 saw that you wanted to merge this today so feel free to ignore, will work on it once merged
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.
Yeah agreed. The formatViemError function came into being after I had done the initial refactoring, so as you mentioned, this is a semi-stale artifact.
Definitely it is worth investing in error handling after this PR goes in.
06f6a0e
to
58263b4
Compare
58263b4
to
f0f7be0
Compare
refactor: different publishers for sequencer and prover feat: sequencer publisher uses forwarder contract
f0f7be0
to
02060aa
Compare
See the design.
TLDR:
Future work: