opnode: Withdrawal E2E test in go by trianglesphere · Pull Request #423 · ethereum-optimism/optimistic-specs

trianglesphere · 2022-05-09T21:04:19Z

Description

This does the following:

Adds withdrawal utilities (to opnode/withdrawals)
Adds an end to end test in go of withdrawals
Adds the L2 withdrawer contract
Updates to a newer version of reference-go-ethereum

Metadata

Fixes ENG-2202

tynes · 2022-05-09T22:51:03Z

opnode/contracts/Makefile

 		--type OptimismPortal \
 		--out ./deposit/deposit_feed_raw.go
+
+


Why exactly is this commented out?

Because the L2 Output Oracle Contract is built in the l2os package. I can remove these fully.

tynes · 2022-05-09T22:55:25Z

Some of this stuff is going to change pending #421 due to it being updated to support backwards compatible withdrawals

codecov-commenter · 2022-05-12T18:40:19Z

Codecov Report

Merging #423 (20143e9) into main (fb044a6) will decrease coverage by 1.35%.
The diff coverage is 28.65%.

@@            Coverage Diff             @@
##             main     #423      +/-   ##
==========================================
- Coverage   51.88%   50.53%   -1.36%     
==========================================
  Files          63       66       +3     
  Lines        6703     7215     +512     
==========================================
+ Hits         3478     3646     +168     
- Misses       2784     3094     +310     
- Partials      441      475      +34

Impacted Files	Coverage Δ
opnode/contracts/withdrawer/withdrawer_raw.go	`4.18% <4.18%> (ø)`
opnode/withdrawals/utils.go	`59.23% <59.23%> (ø)`
opnode/withdrawals/proof.go	`59.70% <59.70%> (ø)`
opnode/test/geth.go	`65.07% <100.00%> (+0.27%)`	⬆️
opnode/test/setup.go	`72.81% <100.00%> (ø)`
l2os/txmgr/txmgr.go	`95.65% <0.00%> (+1.86%)`	⬆️
opnode/contracts/deposit/deposit_feed_raw.go	`11.40% <0.00%> (+6.18%)`	⬆️

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 fb044a6...20143e9. Read the comment docs.

tynes · 2022-05-12T20:04:12Z

opnode/contracts/Makefile

 		--type L1Block \
 		--out ./l1block/l1_block_info_raw.go

+bindings-L2ToL1MessagePasser:


There are some mismatched names in files here, withdrawer -> L2ToL1MessagePasser

TBH I'm ok with the go names being out of date, I've got an issue to update them (ENG-2195)

tynes · 2022-05-12T20:05:24Z

opnode/withdrawals/utils.go

+	"github.com/ethereum/go-ethereum/rpc"
+)
+
+var L2WithdrawalAddr = common.HexToAddress("0x4200000000000000000000000000000000000016")


This is no longer correct, the predeploy is at address(42...0)

Also, this has been removed and I'm using the address in the predeploys package. FYI this address is still found in the typesecript helpers

This is working in the tests because we init the code at predeploy.WithdrawalsAddress and then reference it at that address later.

tynes · 2022-05-12T20:06:12Z

This could use a rebase

trianglesphere · 2022-05-12T20:41:01Z

This could use a rebase

Will do once #416 is merged

This does the following: - Adds withdrawal utilities (to opnode/withdrawals) - Adds an end to end test in go of withdrawals - Adds the L2 withdrawer contract - Updates to a newer version of reference-go-ethereum

protolambda

LGTM! Some things we may want to do in future PRs, nothing blocking though.

protolambda · 2022-05-13T18:00:14Z

opnode/test/system_test.go

+	require.Nil(t, err)
+
+	// Ensure that withdrawal - gas fees are added to the L1 balance
+	// Fun fact, the fee is greater than the withdrawal amount


Fun fact 😅
We're withdrawing only 500 gwei though, so it makes sense

When it's expressed like that, it makes sense. I always forget how may decimals there actually are.

protolambda · 2022-05-13T18:03:47Z

opnode/withdrawals/proof.go

+	"github.com/ethereum/go-ethereum/trie"
+)
+
+type proofDB struct {


For the output root construction we've some proof code already. But without the storage proof part, just the account proof. There it types the rlp nodes as hexutil.Bytes and maintains a map of common.Hash to nodes, instead of using strings for keys. Here it nicely uses the account type + some of the geth rpc client types though. In both cases it does verify correctly, and does verify all the things in the response (i.e. the account details in the response are checked against the rlp node in the proof). Either way, some opportunity for a later PR to take the best of both and reduce code.

protolambda · 2022-05-13T18:05:27Z

opnode/withdrawals/utils.go

+	opts := &bind.CallOpts{Context: ctx}
+	timestampBig := new(big.Int).SetUint64(timestamp)
+
+	portal, err := deposit.NewOptimismPortalCaller(portalAddr, client)


Same applies to deposit as to withdrawer rename I think? Renames in a later PR are fine

protolambda · 2022-05-13T18:07:54Z

opnode/test/system_test.go

+		params.Target,
+		params.Value,
+		params.GasLimit,
+		params.Data,


Might want to test a params.Data that's not empty, but integration tests can pick that up too.

Good call - that'd be part of doing generic contract calls (rather than simple transfer) via the withdrawal mechanism

protolambda · 2022-05-13T18:09:34Z

opnode/test/system_test.go

+// TestWithdrawals checks that a deposit and then withdrawal execution succeeds. It verifies the
+// balance changes on L1 and L2 and has to include gas fees in the balance checks.
+// It does not check that the withdrawal can be executed prior to the end of the finality period.
+func TestWithdrawals(t *testing.T) {


Nice test, long function though. With the monorepo change the e2e tests get their own go module (so we can separate any testing dependencies, and pull in all the other modules), maybe after that we can split the system_test.go in themed files (general, deposits, withdrawals, p2p, etc.) or maybe even packages.

Yea, there's a lot of boilerplate that's hard to avoid.

tynes reviewed May 9, 2022

View reviewed changes

tynes mentioned this pull request May 11, 2022

refactor: new messengers #421

Merged

trianglesphere force-pushed the jg/fix_l2_output_timestamps_v2 branch from 746e1d3 to 65980a2 Compare May 12, 2022 17:40

trianglesphere force-pushed the jg/withdrawal_e2e_test branch from 661e2d3 to 0a3c387 Compare May 12, 2022 18:36

tynes reviewed May 12, 2022

View reviewed changes

Base automatically changed from jg/fix_l2_output_timestamps_v2 to main May 13, 2022 16:54

trianglesphere added 2 commits May 13, 2022 09:55

opnode: Withdrawal E2E test in go

18b87ad

This does the following: - Adds withdrawal utilities (to opnode/withdrawals) - Adds an end to end test in go of withdrawals - Adds the L2 withdrawer contract - Updates to a newer version of reference-go-ethereum

Rely on pre-deploy package

9be0792

trianglesphere force-pushed the jg/withdrawal_e2e_test branch from ef47acf to 9be0792 Compare May 13, 2022 16:55

trianglesphere requested a review from protolambda May 13, 2022 17:20

opnode/test: Remove dead code

20143e9

protolambda approved these changes May 13, 2022

View reviewed changes

tynes approved these changes May 13, 2022

View reviewed changes

trianglesphere merged commit 92fccc2 into main May 13, 2022

trianglesphere deleted the jg/withdrawal_e2e_test branch May 13, 2022 18:16

Conversation

trianglesphere commented May 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tynes commented May 9, 2022

Uh oh!

codecov-commenter commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tynes commented May 12, 2022

Uh oh!

trianglesphere commented May 12, 2022

Uh oh!

protolambda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented May 12, 2022 •

edited

Loading