-
Notifications
You must be signed in to change notification settings - Fork 19
feat: add transaction and address monitoring with real-time updates #191
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
base: main
Are you sure you want to change the base?
feat: add transaction and address monitoring with real-time updates #191
Conversation
Hi @pranavkonde! Great to see so many contributions from you! I haven’t fully reviewed the PR yet, but I’m sharing some feedback so you can start making adjustments in the meantime and we make the most of the time: regarding the Let me know if you need any help! |
Thank you for the feedback! I've moved the transaction monitoring functionality command. The changes are now pushed to the feature branch. Let me know if any adjustments are needed after your review. |
Hi @pranavkonde Please update the And another thing is that when the monitoring is completed, the command does not end (event with ctrl + c) ![]() Please give it a look at how can it be finished effectively |
Hello @chrisarevalo11 Thank you for the feedback! I've moved the transaction monitoring to |
Hello @pranavkonde , kindly follow the next comments.
We'll definitely keep this in mind when it comes time to reward you. |
…ature/transaction-monitoring
Hello @scguaquetam I have made all the suggested changes, Kindly Review it, and let me know, Thanks |
@pranavkonde , I dont see you adjusted your PR to the new structure of our CLI, I can still see the function parameters are not grouped, also , take as reference https://github.com/rsksmart/rsk-cli/blob/main/src/commands/wallet.ts "wallet" module, where we have a logs and spinner specific handling, which I cannot see in your PR, please change it accordingly. |
…ature/transaction-monitoring
I’ve updated the PR as per the suggested changes. |
Hello @pranavkonde I see it much better, thanks, just 2 things.
Please let us know any question you have. |
Thank you for the feedback! I've addressed both points. |
this.startPolling(sessionId); | ||
await this.saveState(); | ||
|
||
console.log(chalk.green(`✅ Started monitoring transaction: ${txHash}`)); |
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.
@pranavkonde the use of console.logs on this class could affect external interactions, please configure here also, the same as you implemented log management on the monitor command, for avoiding console.log on external interactions
I see CodeQL requests haven't been fixed yet , can you take a look? I tagged you on the comments.
|
I have addressed the CodeQL requests, updated the logging in MonitorManager.ts to align with the monitor module, and resolved the Package.json conflict. Please review at your convenience. |
@pranavkonde Please review comments I made, you are not correctly implementing the requested settings on spinner and log functions, also it seems like you are not implementing the monitor functionality inside the |
Thank you for the detailed feedback! I've addressed all the issues mentioned, Can you please take a look now? |
Hello @pranavkonde I can see you followed my comments and now it looks good, thank you so much, I will share this review with people in charge, they will come back to you soon. |
Transaction Monitoring & Alerts Feature
Overview
Implemented a comprehensive transaction and address monitoring system for the rsk-cli tool, allowing users to track transactions and address changes in real-time.
Features Added
Transaction Monitoring
Address Monitoring
Session Management
Error Handling & Validation
CLI Commands
Monitor Transaction
Monitor Address
Session Management