-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Rollup Transaction Handling Improvements #161
Rollup Transaction Handling Improvements #161
Conversation
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 don't have great context here but this LGTM! Generally I see some places in the diff where we have a timestamp
and not a blockNumber
, but I'm guessing that's just because we haven't added contract-side yet.
@@ -11,8 +11,7 @@ const log: Logger = getLogger('block-batch-submitter') | |||
|
|||
export class BlockBatchSubmitter implements BlockBatchListener { | |||
// params: [timestampHex, batchesArrayJSON, signedBatchesArrayJSON] |
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'm missing some context, but seems like this comment/method would need a blockNumberHex
if my intuition serves me right.
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.
Ahh yeah, that's a relic. Gotta update that comment. Nice find!
@@ -86,6 +87,7 @@ const getTransactionResponse = ( | |||
timestamp: number, | |||
data: string, | |||
hash: string, | |||
_gasLimit: number, |
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 noting there's a _
, maybe that's a convention you want though.
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's a gasLimit
file-scoped const that this would shadow and our linter doesn't like that, so I just made it _gasLimit
* Separates storage from SCC and CTC (#151) * First pass version * More minor tweaks for tests to pass * Add authentication * Minor config updates * Fix lint error * Fix naming changes per review * Enable Deployer Whitelist (#119) * first pass, test runner updated * add ability to only validate flag, test passes * all tests passing * clean up console.logs * enforce gas refund preservation * more cleanup/import removal * whitelisted -> allowed * first pass, test runner updated * add ability to only validate flag, test passes * all tests passing * clean up console.logs * enforce gas refund preservation * more cleanup/import removal * whitelisted -> allowed * remove whitespace * Restrict StateTransitionerFactory (#140) * added msg sender check * add create test * cleanup * add param * add addressmanager.address param * CTC Chain Monotonicity Fixes (#93) * [wip] Fix block time logic * some sad path and happy tests passing * more progress * first pass sad cases tested * cleanup, adding empty tests * more reversion tests * rename shouldstartat} * add final couple tests * enable more tests * cleanup * remove .only * textual cleanup * make queue length public * improve structure, comments * update deploy config * address nits Co-authored-by: Karl Floersch <karl@karlfloersch.com> * fix declarations, lint (#152) * Adds river's new Merkle tree implementation, with some cleanup (#148) * Reverts an accidental breaking merge * Added new merkle tree impl * add comments * Final cleanups and merge Co-authored-by: Ben Jones <ben@pseudonym.party> * Fix run gas Lower Bound (#94) * added the check * add test * lower OVM TX size for Kovan * re-remove gas check * update gas vals slightly * lint * lint * Merge master into freeze integration branch (#153) * update solidity version to ^0.7.0 (#122) * update solc version to ^0.7.0 * interfaces back to solidity >0.6.0 <0.8.0 * update solc to 0.7.6 * back to 0.7.4 * upgrade to 0.7.6, fix EXTCODESIZE check * versions >0.5.0 <0.8.0 for xdomain msgers * ctc: disable appendQueueBatch (#150) * ctc: disable appendSequencerBatch * typo: fix * re-enable verifyQueueTransaction test: * add explicit test for verifying queue elements against either append Co-authored-by: Ben Jones <ben@pseudonym.party> * fix up test * remove .only Co-authored-by: Alina <alina@optimism.io> Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com> * add check, simple test, update deploy (#154) * go back to first name (#155) * lint * fix js number error * add error logging to help debug deploy * [code freeze] Fix deploy script (#156) * fix deploy script * add block time config * ensure value is integer * lint * remove console logs from deploy * Moves gas check to applyTransaction (#161) * move to OVM_ST, pass test * remove old test because functionality moved * linting * remove leaf hasing * use safe EXEMRG wrapper (#162) * use safeREQUIRE * add owner getter * relayer: add to config (#160) * relayer: add to config * lint: fix * Fix minor error in test config Co-authored-by: Kelvin Fichter <kelvinfichter@gmail.com> Co-authored-by: ben-chain <ben@pseudonym.party> Co-authored-by: Alina <alina@optimism.io> Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com> Co-authored-by: Kevin Ho <kevinjho1996@gmail.com>
Description
Updates various details of Rollup Transactions, their processing, and the new Web3 API interface for communicating them to L2.
Metadata
Fixes
Contributing Agreement