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

feat: add append_receipts function #8718

Merged
merged 14 commits into from
Jun 13, 2024
Merged

Conversation

kamuik16
Copy link
Contributor

Closes #8466

@mattsse mattsse added C-perf A change motivated by improving speed, memory usage or disk footprint A-db Related to the database labels Jun 10, 2024
@kamuik16
Copy link
Contributor Author

kamuik16 commented Jun 10, 2024

@mattsse @shekhirin ready for your feedback. tracked ci failure and issue seems to be related to #8166.

Copy link
Collaborator

@shekhirin shekhirin left a comment

Choose a reason for hiding this comment

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

Some suggestions

@shekhirin shekhirin added A-static-files Related to static files and removed A-db Related to the database labels Jun 10, 2024
@kamuik16
Copy link
Contributor Author

Some suggestions

if we return an Option, we'll have to do the error handling, so I have i have suggestion, if the receipts are empty, we can simply return the last TxNumber in the statis file, in this way, we also don't return an error and as you said receipts can be empty. wdyt? I have implemented all the suggestions and logic and removed the error ProviderError::EmptyReceipts. lmk wdyt of this approach.

@kamuik16 kamuik16 requested a review from shekhirin June 11, 2024 17:01
Copy link
Collaborator

@shekhirin shekhirin left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for addressing all the comments!

// If receipts are empty, we can simply return the current TxNumber as seen in the static
// file.
if receipts_iter.peek().is_none() {
return Ok(self.writer.user_header().tx_end().expect("qed"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

the CI is failing because it's not guaranteed to have at least one receipt in static files. Let's make this method return Option<TxNumber>.

@shekhirin
Copy link
Collaborator

@mattsse PTAL

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm, ty

@mattsse mattsse added this pull request to the merge queue Jun 13, 2024
Merged via the queue into paradigmxyz:main with commit e5111f0 Jun 13, 2024
29 checks passed
@kamuik16 kamuik16 deleted the kamuik16/8466 branch June 13, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-static-files Related to static files C-perf A change motivated by improving speed, memory usage or disk footprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add append_receipts function
4 participants