Skip to content
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

Track V1 OApps latest configuration #111

Merged
merged 25 commits into from
Mar 7, 2024
Merged

Track V1 OApps latest configuration #111

merged 25 commits into from
Mar 7, 2024

Conversation

sdlyy
Copy link
Member

@sdlyy sdlyy commented Mar 4, 2024

Resolves L2B-4083
Resolves L2B-4095
Resolves L2B-4084

@sdlyy sdlyy self-assigned this Mar 4, 2024
Copy link

vercel bot commented Mar 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
lz-monitoring-frontend ✅ Ready (Inspect) Visit Preview Mar 7, 2024 7:04pm

@sdlyy sdlyy marked this pull request as ready for review March 6, 2024 15:07
const rows = records.map(toRow)
const knex = await this.knex()

await knex('oapp_configuration').delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we clear the table before adding something?

Copy link
Member Author

Choose a reason for hiding this comment

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

leftover from when I used to wipe the whole table instead of using built-in conflict resolver, fixed

return rows.length
}

public async findAll(): Promise<OAppConfigurationRecord[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public async findAll(): Promise<OAppConfigurationRecord[]> {
public async getAll(): Promise<OAppConfigurationRecord[]> {

We follow this convention that find... repository method returns one element, but get... returns multiple.

return rows.length
}

public async findAll(): Promise<OAppDefaultConfigurationRecord[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public async findAll(): Promise<OAppDefaultConfigurationRecord[]> {
public async getAll(): Promise<OAppDefaultConfigurationRecord[]> {

return rows.map(toRecord)
}

public async findBySourceChain(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public async findBySourceChain(
public async getBySourceChain(

Copy link
Contributor

@michalsidzej michalsidzej left a comment

Choose a reason for hiding this comment

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

So far looks good. The logic in the code is quite complicated, testing locally now.

return ids.map((id) => id.id)
}

public async findAll(): Promise<OAppRecord[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public async findAll(): Promise<OAppRecord[]> {
public async getAll(): Promise<OAppRecord[]> {

return rows.map(toRecord)
}

public async findBySourceChain(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public async findBySourceChain(
public async getBySourceChain(

Copy link
Contributor

@michalsidzej michalsidzej left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Couple of nit picks on the way.

'https://l2beat-production.herokuapp.com/api/integrations/layerzero-oapps',
),
tickIntervalMs: env.integer('TRACKING_TICK_INTERVAL_MS', 60_000_000),
rpcUrl: env.string(`${chainNamePrefix}_TRACKING_RPC_URL`),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just use the regular RPC that we already have?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to separate modules + for tip-only data we do not need 'better' providers than those available to the public for now - multicall does its job perfectly fine

@sdlyy sdlyy merged commit e6364e0 into main Mar 7, 2024
5 checks passed
@sdlyy sdlyy deleted the feat/oapps-tracking branch March 7, 2024 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants