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.

Sign up for GitHub

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

Jump to bottom

usePastBlocks flag #151

Merged
merged 18 commits into from
Jun 27, 2023
Merged

usePastBlocks flag #151

merged 18 commits into from
Jun 27, 2023

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Jun 16, 2023

  • Add flag usePastBlocks to Block Trackers
    • When set, allow updating current block to incoming block numbers lower than itself
    • Continue to NOT emit any events when the current block number and incoming block number are the same

This flag is needed to help enable an option to disable nonce caching for specific networks in extension

Ticket: mme-16332

@jiexi jiexi requested a review from a team as a code owner June 16, 2023 17:55
@jiexi jiexi changed the title Use Past Blocks usePastBlocks flag Jun 16, 2023
danfinlay
danfinlay previously approved these changes Jun 16, 2023
View reviewed changes
Copy link
Contributor

@danfinlay danfinlay left a comment

Choose a reason for hiding this comment

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

I understand why this is needed, it makes sense to me. It feels a bit hacky, but it does satisfy the goal of this feature, which is a good one.

I'm not sure how I'd approach it differently, so I would not block. It's great to see progress on this feature!

@jiexi
Copy link
Contributor Author

jiexi commented Jun 20, 2023

Comments addressed. Hopefully the way I organized the new tests seems sane. @danfinlay @mcmire

mcmire
mcmire reviewed Jun 22, 2023
View reviewed changes
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

This is great! Just a couple more suggestions, but it's looking perfect after that.

@jiexi jiexi force-pushed the jl/mme-16332/nonce-cache-dev-mode branch from 8b88aab to aada873 Compare June 23, 2023 20:52
@jiexi jiexi force-pushed the jl/mme-16332/nonce-cache-dev-mode branch from aada873 to 7b17df8 Compare June 23, 2023 20:56
@jiexi jiexi requested review from danfinlay and mcmire June 26, 2023 16:40
mcmire
mcmire approved these changes Jun 27, 2023
View reviewed changes
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Looks great now! Thanks for your patience in working with me on this.

@jiexi jiexi merged commit 5beadc4 into main Jun 27, 2023
@jiexi jiexi deleted the jl/mme-16332/nonce-cache-dev-mode branch June 27, 2023 20:22
@jiexi jiexi mentioned this pull request Jun 29, 2023
Draft
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcmire mcmire mcmire approved these changes

@danfinlay danfinlay Awaiting requested review from danfinlay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jiexi @mcmire @danfinlay