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

Replace explorer prefix search with exact match for block and transaction hash #722

Merged
merged 3 commits into from
Oct 30, 2024

Conversation

imabdulbasit
Copy link
Contributor

addresses https://github.com/EspressoSystems/espresso-block-explorer/issues/148

This PR:

  • replaces the prefix search with equality for block and transaction searches
  • replace search query parameter type from String to TaggedBase64
  • queries database for blocks or transactions based on the tag

@imabdulbasit imabdulbasit requested a review from Ayiga October 29, 2024 17:32
@imabdulbasit imabdulbasit marked this pull request as draft October 29, 2024 18:01
@imabdulbasit imabdulbasit changed the title replace prefix search with equality Replace explorer prefix search with exact match for block and transaction hash Oct 29, 2024
@imabdulbasit imabdulbasit marked this pull request as ready for review October 29, 2024 18:10
Copy link
Member

@jbearer jbearer left a comment

Choose a reason for hiding this comment

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

@Ayiga can you confirm this will work with the block explorer?

Comment on lines 534 to 538
LIMIT 5"
);
let block_query_rows = query(block_query.as_str())
.bind(&search_query_string)
.fetch(self.as_mut());
Copy link
Member

Choose a reason for hiding this comment

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

We can do LIMIT 1 and fetch_one here, querying for blocks by equality on hash is only ever going to return one block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah

Copy link
Contributor Author

@imabdulbasit imabdulbasit Oct 29, 2024

Choose a reason for hiding this comment

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

I am considering making the blocks field an Option<BlockSummary> since there can only be one block now. However, I don't want to break the frontend, and I also think that SearchResult should be an enum

Copy link
Member

Choose a reason for hiding this comment

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

Making this return an enum would indeed be a backwards incompatible change. I can possibly support it with a backwards compatible decoder that can support either representation, but I think that would need to be in place in the Block Explorer client-side code before that sort of change is introduced on the server side.

@imabdulbasit imabdulbasit marked this pull request as draft October 29, 2024 19:54
@imabdulbasit imabdulbasit marked this pull request as ready for review October 29, 2024 20:20
@Ayiga
Copy link
Member

Ayiga commented Oct 30, 2024

@Ayiga can you confirm this will work with the block explorer?

I think this should be fine in terms of functionality at the moment. There is a slight change in the input requirements that may cause problems with the current version of the Block Explorer. That being the requirement of the input to be a TaggedBase64. Though I do think that this is a good change, it does mean that the unchanged search request will start returning 4xx errors where it before didn't.

But this is likely alright, as we'll want to change the Block Explorer's search behavior regardless so that it isn't submitting these requests when they don't conform to the TaggedBase64 requirements.

@imabdulbasit imabdulbasit merged commit 509ba8c into main Oct 30, 2024
8 checks passed
@imabdulbasit imabdulbasit deleted the ab/rm-prefix-search branch October 30, 2024 14:47
@Ayiga
Copy link
Member

Ayiga commented Oct 30, 2024

Just tested this. The 4xx errors aren't affecting the current block explorer in any meaningful way. So this should be relatively safe. Though, gain we'd want to update the logic in the block explorer to not send partial TaggedBase64 search terms.

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.

3 participants