-
Notifications
You must be signed in to change notification settings - Fork 45
fix: new block events logic [wip] #3120
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
fix: new block events logic [wip] #3120
Conversation
Summary of ChangesHello @shanimal08, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request re-implements the core logic for handling new block events within the application, focusing on a more robust and centralized approach for wallet-related updates. It standardizes the transaction data used in events, streamlines the emission of new block mined events, and enhances the wallet's ability to track and react to coinbase transactions. These changes aim to improve the accuracy and responsiveness of block height and balance updates across the application, particularly for mining-related features. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the block event handling by replacing TransactionInfo with DisplayedTransaction and renaming NewBlockHeightPayload to NewBlockMined. The logic for detecting new blocks and associated coinbase transactions has been moved from events_manager to the balance_tracker in the wallet, which is a good architectural improvement. The frontend has also been updated to align with these backend changes, including a new synchronization mechanism in useBlockTip.
While the direction of these changes is positive, I've identified a few areas for improvement. There are several debug log messages that should be removed. More importantly, there's a potential logic issue in balance_tracker that could prevent new block events from being emitted correctly, and a potential for an infinite refetch loop in the useBlockTip hook on the frontend. I've provided specific comments and suggestions to address these points. Given this is a work-in-progress, I hope this feedback is helpful for finalizing the feature.
…ement emits .. wip: clean up useBlockTip for new block timing on the animation .. wip: rm unused/uneeded code
a3d1d67 to
a3c466d
Compare
|
closing & will re-open a new one as the direction changed (re. coinbases txs/solo mining animations) |
Description
(still WIP)
Motivation and Context
How Has This Been Tested?