Skip to content

l2geth: allow for unprotected txs#1720

Merged
tynes merged 7 commits intodevelopfrom
feat/allow-unprotected-txs
Nov 30, 2021
Merged

l2geth: allow for unprotected txs#1720
tynes merged 7 commits intodevelopfrom
feat/allow-unprotected-txs

Conversation

@tynes
Copy link
Contributor

@tynes tynes commented Nov 8, 2021

Description

The old transaction batch serialization assumed that the
correct chain id was used as there was a custom transaction
serialization used. Now that the full RLP of the transactions
are submitted to L1 for availability, it is possible to enable
non EIP155 transactions. This allows for universal addresses
to be used, where the same address exists across many chains.

A test is updated to test specifically that transactions that
are not protected with a chain id can be submitted and accepted
by the sequencer.

Fixes ENG-1680

@changeset-bot
Copy link

changeset-bot bot commented Nov 8, 2021

🦋 Changeset detected

Latest commit: 6bd6768

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@eth-optimism/data-transport-layer Patch
@eth-optimism/integration-tests Patch
@eth-optimism/l2geth Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added 2-reviewers A-integration Area: integration tests A-cannon Area: cannon labels Nov 8, 2021
@tynes
Copy link
Contributor Author

tynes commented Nov 8, 2021

This PR will also need an update to the DTL

@github-actions github-actions bot added the M-dtl label Nov 8, 2021
@tynes
Copy link
Contributor Author

tynes commented Nov 8, 2021

This PR will also need an update to the DTL

Added in e84114d

@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2021

Codecov Report

Merging #1720 (50feea3) into develop (b6bdea0) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1720      +/-   ##
===========================================
- Coverage    72.00%   71.99%   -0.01%     
===========================================
  Files           70       70              
  Lines         2318     2321       +3     
  Branches       345      346       +1     
===========================================
+ Hits          1669     1671       +2     
- Misses         649      650       +1     
Flag Coverage Δ
batch-submitter 62.07% <ø> (ø)
contracts 87.96% <ø> (ø)
core-utils 57.50% <ø> (ø)
data-transport-layer 38.64% <75.00%> (+0.41%) ⬆️
message-relayer 70.86% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/data-transport-layer/src/utils/eth-tx.ts 85.71% <75.00%> (-14.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6bdea0...50feea3. Read the comment docs.

Copy link
Contributor

@annieke annieke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

@tynes tynes force-pushed the feat/allow-unprotected-txs branch from e84114d to e579cf2 Compare November 8, 2021 22:53
@github-actions github-actions bot added the A-ops Area: ops label Nov 8, 2021
@tynes
Copy link
Contributor Author

tynes commented Nov 8, 2021

e579cf2 adds test coverage for replicas, it enables the replica in the integration tests which adds a bit of additional load to the machine that has to run the integration tests

@tynes tynes force-pushed the feat/allow-unprotected-txs branch 3 times, most recently from 65cd247 to 2232368 Compare November 9, 2021 00:29
smartcontracts
smartcontracts previously approved these changes Nov 9, 2021
Copy link
Contributor

@smartcontracts smartcontracts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@smartcontracts
Copy link
Contributor

Actually there are failing integration tests that need to be solved first

@smartcontracts smartcontracts dismissed their stale review November 9, 2021 17:39

Blocking merge

Copy link
Contributor

@smartcontracts smartcontracts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking merge of this PR pending additional tests, will have to wait until after upgrade

Base automatically changed from regenesis/0.5.0 to develop November 10, 2021 22:08
@smartcontracts smartcontracts force-pushed the feat/allow-unprotected-txs branch from 2232368 to f541767 Compare November 17, 2021 21:48
@tynes tynes force-pushed the feat/allow-unprotected-txs branch 3 times, most recently from 2f246ac to 1900e61 Compare November 19, 2021 20:01
@tynes

This comment has been minimized.

@github-actions github-actions bot added the M-ci Meta: ci related work label Nov 29, 2021
tynes and others added 5 commits November 29, 2021 14:05
The old transaction batch serialization assumed that the
correct chain id was used as there was a custom transaction
serialization used. Now that the full RLP of the transactions
are submitted to L1 for availability, it is possible to enable
non EIP155 transactions. This allows for universal addresses
to be used, where the same address exists across many chains.

A test is updated to test specifically that transactions that
are not protected with a chain id can be submitted and accepted
by the sequencer.
When syncing from L2, all transactions were assumed to be
protected. To prevent a network split for replicas,
the DTL needs to check for transactions that are not
protected and handle parsing the `v` parameter
appropriately.
@tynes tynes force-pushed the feat/allow-unprotected-txs branch from c484d5a to 300683c Compare November 29, 2021 22:05
@tynes tynes force-pushed the feat/allow-unprotected-txs branch from fe6a757 to 6bd6768 Compare November 30, 2021 00:10
@tynes tynes merged commit cc02fdf into develop Nov 30, 2021
@tynes tynes deleted the feat/allow-unprotected-txs branch November 30, 2021 00:14
@hellwolf
Copy link

hellwolf commented Dec 1, 2021

Great!

how could we utilize this to deploy the 1820 contract? https://kovan-optimistic.etherscan.io/address/0x1820a4B7618BdE71Dce8cdc73aAB6C95905faD24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cannon Area: cannon A-integration Area: integration tests A-ops Area: ops M-ci Meta: ci related work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants