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

Update APIs with new scroll DB #141

Merged
merged 13 commits into from
Jul 12, 2023
Merged

Update APIs with new scroll DB #141

merged 13 commits into from
Jul 12, 2023

Conversation

silathdiir
Copy link
Contributor

@silathdiir silathdiir commented Jun 30, 2023

Summary

Update APIS as:

  • Get /api/batch?index=BATCH_INDEX

  • Get /api/batches?page=PAGE&per_page=PER_PAGE

  • Get /api/batch_blocks?batch_index=BATCH_INDEX

  • Get /api/chunks?batch_index=BATCH_INDEX

  • Get /api/chunk_blocks?chunk_index=CHUNK_INDEX

  • Get /api/last_batch_indexes

  • Get /api/search?keyword=KEYWORD

Could reference detailed specific requests and reponses in docs/openapi.spec.

Test

Update db/tests/test.sql for fake data, and test on local (http://localhost:5001/).


@silathdiir silathdiir marked this pull request as draft July 2, 2023 13:30
@silathdiir silathdiir requested a review from colinlyguo July 7, 2023 07:37
@silathdiir silathdiir changed the title [WIP] add chunk model Update APIs with new scroll DB Jul 7, 2023
@silathdiir silathdiir marked this pull request as ready for review July 7, 2023 13:53
@silathdiir silathdiir requested review from OrestTa and Thegaram July 7, 2023 13:53
@@ -4,4 +4,5 @@ updates:
directory: "/"
schedule:
interval: weekly
open-pull-requests-limit: 5
# Disable updates
open-pull-requests-limit: 0
Copy link
Contributor Author

@silathdiir silathdiir Jul 10, 2023

Choose a reason for hiding this comment

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

Suppose to disable dependabot (auto-updagrde-dependencies) for a while 😂.
Will enable again until upgrading rust-toolchain and fix issue #107.

0xmountaintop
0xmountaintop previously approved these changes Jul 11, 2023
Copy link
Contributor

@0xmountaintop 0xmountaintop left a comment

Choose a reason for hiding this comment

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

LGTM. and we can add AND deleted_at IS NULL to all queries.

colinlyguo
colinlyguo previously approved these changes Jul 11, 2023
@silathdiir silathdiir dismissed stale reviews from colinlyguo and 0xmountaintop via d76ab85 July 11, 2023 11:20
Copy link
Contributor

@0xmountaintop 0xmountaintop left a comment

Choose a reason for hiding this comment

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

there are further schema changes in scroll-tech/scroll#633

@silathdiir
Copy link
Contributor Author

there are further schema changes in scroll-tech/scroll#633

Copy from scroll and update in commit c3fa8f0.

@silathdiir
Copy link
Contributor Author

LGTM. and we can add AND deleted_at IS NULL to all queries.

Add deleted_at IS NULL fixes in commit 8b3680a.

colinlyguo
colinlyguo previously approved these changes Jul 11, 2023
"SELECT start_block_number FROM {} where index = $1",
table_name::CHUNK
"SELECT start_block_number FROM {}
where index = $1",
Copy link
Contributor

Choose a reason for hiding this comment

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

deleted_at

Copy link
Contributor Author

@silathdiir silathdiir Jul 11, 2023

Choose a reason for hiding this comment

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

Sorry for not explaining this.

Just thought, could the start (and end) chunk (and block) be marked as deleted?
Or the start_chunk_index (and others) should be updated to batch table if the start chunk is marked as deleted (Seems no).

If so, seems I should not use start_chunk_index, end_chunk_index, start_block_number and end_block_number (in this backend).
Should I fetch start_chunk_index as (if this start chunk is marked as deleted):

select min(index) from chunk where batch_hash = XXX and deleted_at is null;

WDYT? Thanks.

Copy link
Contributor Author

@silathdiir silathdiir Jul 11, 2023

Choose a reason for hiding this comment

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

If limit deleted_at is NULL in this SQL, it may be NULL here 😂 (to use start_chunk_index to get start_block_number).

And break the process to fetch batch, Or I should use above SQL for start_chunk_index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussing with @HAOYUatHZ , in commit 3b99586, fix to use chunk hash to query start and end block number, and add deleted_at IS NULL condition.

Tested on local with db/tests/test.sql.

@@ -60,7 +64,8 @@ pub async fn get_end_block_number_by_index(
chunk_index: i64,
) -> Result<Option<i64>> {
let stmt = format!(
"SELECT end_block_number FROM {} where index = $1",
"SELECT end_block_number FROM {}
WHERE index = $1",
Copy link
Contributor

Choose a reason for hiding this comment

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

deleted_at

@silathdiir silathdiir requested a review from 0xmountaintop July 11, 2023 14:30
…mber`, and add `deleted_at IS NULL` condition.
@0xmountaintop 0xmountaintop merged commit f4ff0ed into develop Jul 12, 2023
@0xmountaintop 0xmountaintop deleted the add-chunk-model branch July 12, 2023 08:20
@OrestTa
Copy link
Contributor

OrestTa commented Jul 12, 2023

@HAOYUatHZ deployed to staging?

@silathdiir
Copy link
Contributor Author

@HAOYUatHZ deployed to staging?

I have asked @Thegaram for helping deploy this (after deploying scroll DB). Thanks 🙏

@Thegaram
Copy link

deployed to staging?

Cannot deploy on staging, it's incompatible with pre-sepolia DB versions.

@OrestTa
Copy link
Contributor

OrestTa commented Jul 12, 2023

So we will be testing this PR alongside scroll-tech/frontends#415 on the Sepolia testnet?

@Thegaram
Copy link

So we will be testing this PR alongside scroll-tech/frontends#415 on the Sepolia testnet?

It would be best to test it with a local testnet first, running this version: https://github.com/scroll-tech/testnet/pull/632

@OrestTa
Copy link
Contributor

OrestTa commented Jul 27, 2023

Where is this currently deployed to?

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.

5 participants