Skip to content
This repository was archived by the owner on Apr 11, 2021. It is now read-only.

Adds L1RollupTxId on Transaction and Transaction-related objects#10

Merged
willmeister merged 2 commits intomasterfrom
YAS-550/PassingL1RollupTxIdAndChangingBlockBatches
Jul 31, 2020
Merged

Adds L1RollupTxId on Transaction and Transaction-related objects#10
willmeister merged 2 commits intomasterfrom
YAS-550/PassingL1RollupTxIdAndChangingBlockBatches

Conversation

@willmeister
Copy link
Copy Markdown
Contributor

Description

Adds L1RollupTxId on Transactions and adjusts the SendBlockBatches endpoint to be SendRollupTransactions to make more sense.

Metadata

Fixes

Contributing Agreement

Copy link
Copy Markdown
Contributor

@karlfloersch karlfloersch left a comment

Choose a reason for hiding this comment

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

Nice! It seems like it's not actually hard to update a crazy number of values all at once in Geth! I still need to review the Typescript side of things but this side looks super non-controversially reasonable.

Left a question but overall this PR LGTM! Woot! And big fan of the naming

Comment thread cmd/geth/retesteth.go

func (api *RetestethAPI) BlockNumber(ctx context.Context) (uint64, error) {
//fmt.Printf("BlockNumber, response: %d\n", api.blockNumber)
//fmt.Printf("SubmissionNumber, response: %d\n", api.blockNumber)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What was the reason for the rename?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ahh I see now -- renaming blockNumber to SubmissionNumber -- nice! Agreed it seems like a better word

Comment thread internal/ethapi/api.go
Timestamp *hexutil.Uint64 `json:"timestamp"`
BlockNumber *hexutil.Uint64 `json:"blockNumber"`
Batches [][]*RollupTransaction `json:"batches"`
type GethSubmission struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!

Copy link
Copy Markdown
Contributor

@karlfloersch karlfloersch Jul 30, 2020

Choose a reason for hiding this comment

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

Am I right that the L1RollupTxId is what allows us to tie L1->L2 txs for instance to the outputs of Geth? And we need it because the tx hash isn't valid if the tx that gets processed can't be decoded?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes on the first part, and yeah, that's one of the reasons on the second part. We need a unique identifier for transactions we pull from L1 queues, and unfortunately tx hash can't be. Like you said, someone could submit an incorrectly formatted and therefore invalid transaction, someone could submit the exact same transaction twice, etc.

As a nice plus, presence or absence of this field in an L2 Tx Output lets us know with certainty where the tx originated (in an L1 queue or from a tx that has never been posted to L1, respectively)

…pi.go. Signing key and endpoint will be removed when go handles batch fetching
@willmeister willmeister merged commit 499c6b5 into master Jul 31, 2020
@willmeister willmeister deleted the YAS-550/PassingL1RollupTxIdAndChangingBlockBatches branch July 31, 2020 13:55
karlfloersch pushed a commit that referenced this pull request Nov 20, 2020
* Adding L1RollupTxId field to Transactions
* Adding rollup transactions signing key config and bug fixing within api.go. Signing key and endpoint will be removed when go handles batch fetching
karlfloersch pushed a commit that referenced this pull request Nov 20, 2020
* Adding L1RollupTxId field to Transactions
* Adding rollup transactions signing key config and bug fixing within api.go. Signing key and endpoint will be removed when go handles batch fetching
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants