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

Advanced Querying for ChainReader #14511

Merged
merged 10 commits into from
Sep 28, 2024
Merged

Conversation

silaslenihan
Copy link
Contributor

@silaslenihan silaslenihan commented Sep 20, 2024

Copy link
Contributor

github-actions bot commented Sep 26, 2024

Solidity Review Jira issue

Hey! We have taken the liberty to link this PR to a Jira issue for Solidity Review.

This is a new feature, that's currently in the pilot phase, so please make sure that the linkage is correct. In a contrary case, please update it manually in JIRA and replace Solidity Review issue key in the changeset file with the correct one.
Please reach out to the Test Tooling team and notify them about any issues you encounter.

Any changes to the Solidity Review Jira issue should be reflected in the changeset file. If you need to update the issue key, please do so manually in the following changeset file: contracts/.changeset/tall-donkeys-flow.md

This PR has been linked to Solidity Review Jira issue: BCFR-957

@silaslenihan silaslenihan changed the title [DRAFT] Advanced Querying for ChainReader Advanced Querying for ChainReader Sep 26, 2024
@silaslenihan silaslenihan marked this pull request as ready for review September 26, 2024 15:02
@silaslenihan silaslenihan removed the request for review from a team September 26, 2024 15:02
Comment on lines 552 to 553
if r := recover(); r != nil {
err = fmt.Errorf("%w: cannot encode %s data word comparator", commontypes.ErrInvalidType, dwTypeID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we expecting a panic?

Copy link
Contributor

Choose a reason for hiding this comment

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

I added this, geth abi package in some scenarios panics instead of throwing an error

Copy link
Contributor

Choose a reason for hiding this comment

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

How about mentioning "recovered from panic" somewhere in the returned error? And r is critical to include as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the error message accordingly.

@cl-sonarqube-production
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
17.9% Duplication on New Code (required ≤ 10%)

See analysis details on SonarQube

@ilija42 ilija42 added this pull request to the merge queue Sep 28, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 28, 2024
@ilija42 ilija42 added this pull request to the merge queue Sep 28, 2024
Merged via the queue into develop with commit 8fa9a67 Sep 28, 2024
137 of 138 checks passed
@ilija42 ilija42 deleted the BCFR-44-Advanced-CR-Querying branch September 28, 2024 08:02
AnieeG pushed a commit that referenced this pull request Sep 30, 2024
* Advanced Querying for ChainReader

* Handle pointer type value comparator encoding

* Handle geth abi panic when encoding data word value comparators

* fix linting issues

* [Bot] Update changeset file with jira issues

* fix linting issues

* [Bot] Update changeset file with jira issues

* fix linting issues

* [Bot] Update changeset file with jira issues

* changed function name and added comments

---------

Co-authored-by: ilija <[email protected]>
Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
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.

4 participants