Skip to content

dtl: add query param based querying for txs #479

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.

Sign up for GitHub

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

Merged
gakonst merged 6 commits into from
Apr 21, 2021
Merged

dtl: add query param based querying for txs #479

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0491ff0
dtl: remove stopL2SyncAtHeight config
tynes Apr 20, 2021
5f6cf32
dtl: implement query param based backend
tynes Apr 20, 2021
79df951
dtl: remove dead comment
tynes Apr 20, 2021
f5b8763
dtl: add changeset
tynes Apr 20, 2021
2e92209
dtl: standardize on backend query param
tynes Apr 21, 2021
e86ee09
changeset: update comment
tynes Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/calm-books-hope.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@eth-optimism/data-transport-layer": patch
---

Add backwards compatible query params to REST API

@gakonst gakonst Apr 21, 2021

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.

Nit: May want to make your message a bit clearer, I do not understand what you changed just by reading this.

Something like: Allow the DTL to provide data from either L1 or L2, configurable via a query param sent by the client.

29 changes: 1 addition & 28 deletions packages/data-transport-layer/src/services/l2-ingestion/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,6 @@ const optionSettings = {
default: false,
validate: validators.isBoolean,
},
stopL2SyncAtBlock: {
default: Infinity,
validate: validators.isInteger,
},
}

export class L2IngestionService extends BaseService<L2IngestionServiceOptions> {
Expand Down Expand Up @@ -80,30 +76,7 @@ export class L2IngestionService extends BaseService<L2IngestionServiceOptions> {
const highestSyncedL2BlockNumber =
(await this.state.db.getHighestSyncedUnconfirmedBlock()) || 1

// Shut down if we're at the stop block.
if (
this.options.stopL2SyncAtBlock !== undefined &&
this.options.stopL2SyncAtBlock !== null &&
highestSyncedL2BlockNumber >= this.options.stopL2SyncAtBlock
) {
this.logger.info(
"L2 sync is shutting down because we've reached your target block. Goodbye!"
)
return
}

let currentL2Block = await this.state.l2RpcProvider.getBlockNumber()

// Make sure we can't exceed the stop block.
if (
this.options.stopL2SyncAtBlock !== undefined &&
this.options.stopL2SyncAtBlock !== null
) {
currentL2Block = Math.min(
currentL2Block,
this.options.stopL2SyncAtBlock
)
}
const currentL2Block = await this.state.l2RpcProvider.getBlockNumber()

// Make sure we don't exceed the tip.
const targetL2Block = Math.min(
Expand Down
3 changes: 1 addition & 2 deletions packages/data-transport-layer/src/services/main/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,12 @@ export interface L1DataTransportServiceOptions {
logsPerPollingInterval: number
pollingInterval: number
port: number
showUnconfirmedTransactions: boolean
syncFromL1?: boolean
syncFromL2?: boolean
transactionsPerPollingInterval: number
legacySequencerCompatibility: boolean
stopL2SyncAtBlock?: number
sentryDsn?: string
defaultSource: string
}

const optionSettings = {
Expand Down
3 changes: 1 addition & 2 deletions packages/data-transport-layer/src/services/run.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ interface Bcfg {
l2ChainId: config.uint('l2ChainId'),
syncFromL1: config.bool('syncFromL1', true),
syncFromL2: config.bool('syncFromL2', false),
showUnconfirmedTransactions: config.bool('syncFromL2', false),
transactionsPerPollingInterval: config.uint(
'transactionsPerPollingInterval',
1000
Expand All @@ -48,8 +47,8 @@ interface Bcfg {
'legacySequencerCompatibility',
false
),
stopL2SyncAtBlock: config.uint('stopL2SyncAtBlock'),
sentryDsn: config.str('sentryDsn'),
defaultSource: config.str('defaultSource', 'batched'),
})

await service.start()
Expand Down
130 changes: 75 additions & 55 deletions packages/data-transport-layer/src/services/server/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,12 @@ const optionSettings = {
return validators.isUrl(val) || validators.isJsonRpcProvider(val)
},
},
showUnconfirmedTransactions: {
validate: validators.isBoolean,
},
defaultSource: {
default: 'batched',
validate: (val: string) => {
return val === 'batched' || val === 'sequenced'

@annieke annieke Apr 19, 2021

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.

will we have more sources than batched or sequenced in the future? assuming no since it seems like they directly correspond to l1 and l2 right?

@annieke annieke Apr 19, 2021

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.

if we anticipate adding more sources

['batched', 'sequenced'].includes(val)

@tynes tynes Apr 20, 2021

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.

They don't necessarily correspond to L1 and L2 per say, really the abstraction is batched or batched + pre-batched. The batched transactions are currently batched to Ethereum L1 but will be batched to an Eth2 data shard in the future.

Working on unifying the language between the different codebases.

['batched', 'sequenced'].includes(val)

We only have 2 sources right now so I think its fine. I prefer writing JS using a subset of the language that is as C like as possible with Python sprinkled in, see https://github.com/handshake-org/hsd/blob/master/lib/blockchain/chain.js as an example

}
}
}

export class L1TransportServer extends BaseService<L1TransportServerOptions> {
Expand All @@ -66,7 +69,6 @@ export class L1TransportServer extends BaseService<L1TransportServerOptions> {
} = {} as any

protected async _init(): Promise<void> {
// TODO: I don't know if this is strictly necessary, but it's probably a good thing to do.
if (!this.options.db.isOpen()) {
await this.options.db.open()
}
Expand Down Expand Up @@ -171,14 +173,26 @@ export class L1TransportServer extends BaseService<L1TransportServerOptions> {
* TODO: Link to our API spec.
*/
private _registerAllRoutes(): void {
// TODO: Maybe add doc-like comments to each of these routes?

this._registerRoute(
'get',

@tynes tynes Apr 16, 2021 •
edited
Loading

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.

@smartcontracts Any thoughts on how to reduce complexity in this endpoint? We need to add the backend query param to this so that it can determine whether or not its sync'd L1 or L2. Ideally we refactor the db handlers now that we are moving away from confirmed/not confirmed as concepts in this codebase

@annieke annieke Apr 19, 2021

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.

is backend in this case defaultSource or a different concept?

@gakonst gakonst Apr 21, 2021

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.

Based on the code, I believe it's the same thing @annieke. @tynes may want to use consistent language and call it backend everywhere.

@tynes what do you think is too complex here? This looks OK to me.

@tynes tynes Apr 21, 2021

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.

what do you think is too complex here? This looks OK to me.

I'm talking about the crazy if/else nesting
https://github.com/ethereum-optimism/optimism/pull/479/files#diff-29a43f33722b902c31ee7224a372262d2b73f170ed0af5be6aca1b54aa505a72L183-L211

'/eth/syncing',
async (): Promise<SyncingResponse> => {
const highestL2BlockNumber = await this.state.db.getHighestL2BlockNumber()
const currentL2Block = await this.state.db.getLatestTransaction()
async (req): Promise<SyncingResponse> => {
const source = req.query.source || this.options.defaultSource

let currentL2Block
let highestL2BlockNumber
switch (source) {
case 'batched':
currentL2Block = await this.state.db.getLatestTransaction()
highestL2BlockNumber = await this.state.db.getHighestL2BlockNumber()
break
case 'sequenced':
currentL2Block = await this.state.db.getLatestUnconfirmedTransaction()
highestL2BlockNumber = await this.state.db.getHighestSyncedUnconfirmedBlock()
break
default:
throw new Error(`Unknown transaction source ${source}`)
}

if (currentL2Block === null) {
if (highestL2BlockNumber === null) {
Expand Down Expand Up @@ -329,21 +343,19 @@ export class L1TransportServer extends BaseService<L1TransportServerOptions> {
this._registerRoute(
'get',
'/transaction/latest',
async (): Promise<TransactionResponse> => {
let transaction = await this.state.db.getLatestFullTransaction()
if (this.options.showUnconfirmedTransactions) {
const latestUnconfirmedTx = await this.state.db.getLatestUnconfirmedTransaction()
if (
transaction === null ||
transaction === undefined ||
latestUnconfirmedTx.index >= transaction.index
) {
transaction = latestUnconfirmedTx
}
}
async (req): Promise<TransactionResponse> => {
const source = req.query.source || this.options.defaultSource
let transaction = null

if (transaction === null) {
transaction = await this.state.db.getLatestFullTransaction()
switch (source) {
case 'batched':
transaction = await this.state.db.getLatestFullTransaction()
break
case 'sequenced':
transaction = await this.state.db.getLatestUnconfirmedTransaction()
break
default:
throw new Error(`Unknown transaction source ${source}`)
}

if (transaction === null) {
Expand All @@ -368,17 +380,22 @@ export class L1TransportServer extends BaseService<L1TransportServerOptions> {
'get',
'/transaction/index/:index',
async (req): Promise<TransactionResponse> => {
const source = req.query.source || this.options.defaultSource
let transaction = null
if (this.options.showUnconfirmedTransactions) {
transaction = await this.state.db.getUnconfirmedTransactionByIndex(
BigNumber.from(req.params.index).toNumber()
)
}

if (transaction === null) {
transaction = await this.state.db.getFullTransactionByIndex(
BigNumber.from(req.params.index).toNumber()
)
switch (source) {
case 'batched':
transaction = await this.state.db.getFullTransactionByIndex(
BigNumber.from(req.params.index).toNumber()
)
break
case 'sequenced':
transaction = await this.state.db.getUnconfirmedTransactionByIndex(
BigNumber.from(req.params.index).toNumber()
)
break
default:
throw new Error(`Unknown transaction source ${source}`)
}

if (transaction === null) {
Expand Down Expand Up @@ -456,21 +473,19 @@ export class L1TransportServer extends BaseService<L1TransportServerOptions> {
this._registerRoute(
'get',
'/stateroot/latest',
async (): Promise<StateRootResponse> => {
let stateRoot = await this.state.db.getLatestStateRoot()
if (this.options.showUnconfirmedTransactions) {
const latestUnconfirmedStateRoot = await this.state.db.getLatestUnconfirmedStateRoot()
if (
stateRoot === null ||
stateRoot === undefined ||
latestUnconfirmedStateRoot.index >= stateRoot.index
) {
stateRoot = latestUnconfirmedStateRoot
}
}
async (req): Promise<StateRootResponse> => {
const source = req.query.source || this.options.defaultSource
let stateRoot = null

if (stateRoot === null) {
stateRoot = await this.state.db.getLatestStateRoot()
switch (source) {
case 'batched':
stateRoot = await this.state.db.getLatestStateRoot()
break
case 'sequenced':
stateRoot = await this.state.db.getLatestUnconfirmedStateRoot()
break
default:
throw new Error(`Unknown transaction source ${source}`)
}

if (stateRoot === null) {
Expand All @@ -495,17 +510,22 @@ export class L1TransportServer extends BaseService<L1TransportServerOptions> {
'get',
'/stateroot/index/:index',
async (req): Promise<StateRootResponse> => {
const source = req.query.source || this.options.defaultSource
let stateRoot = null
if (this.options.showUnconfirmedTransactions) {
stateRoot = await this.state.db.getUnconfirmedStateRootByIndex(
BigNumber.from(req.params.index).toNumber()
)
}

if (stateRoot === null) {
stateRoot = await this.state.db.getStateRootByIndex(
BigNumber.from(req.params.index).toNumber()
)
switch (source) {
case 'batched':
stateRoot = await this.state.db.getStateRootByIndex(
BigNumber.from(req.params.index).toNumber()
)
break
case 'sequenced':
stateRoot = await this.state.db.getUnconfirmedStateRootByIndex(
BigNumber.from(req.params.index).toNumber()
)
break
default:
throw new Error(`Unknown transaction source ${source}`)
}

if (stateRoot === null) {
Expand Down