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:show transaction history for current wallet #41

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SarveshLimaye
Copy link

@SarveshLimaye SarveshLimaye commented Dec 6, 2024

  • Added functionality to fetch wallet transaction history

@ezequiel-rodriguez @chrisarevalodev Could you pls take a look ?

Screenshot 2024-12-06 at 11 19 58 PM

Screenshot 2024-12-06 at 11 20 24 PM

@ezequiel-rodriguez
Copy link
Collaborator

Hey @SarveshLimaye,
please submit your contribution in the following form in order to get a reward for it.
Thank you

@ezequiel-rodriguez
Copy link
Collaborator

Hey,
First of all, thanks for all the hard working you are doing and all your contributions.

Regarding this new feature, here is some feedback:

  • Alchemy key being asked each time, it is not a great UX. My suggestion is that it can be included as a parameter in the command. This way it can be reused each time the user call the command

  • The response from alchemy being thrown directly to the output as a json is not cool. Check the other commands as examples. In case for special fields or to have some flexibility if alchemy changes their response, you can add a section where you parse each pair of parameter/value and show them as they are.

@SarveshLimaye
Copy link
Author

@ezequiel-rodriguez Thank you so much for reviewing the changes and sharing feedback.

I have tried to address your feedback -

  • Now, I am taking the alchemy key from the parameter, saving it in the rootstock-wallet.json file, and using it for further calls.
  • Instead of returning a response directly from Alchemy api, I am parsing each pair of parameter/value and returning it to the user

It would be great if you could review the changes.

Thank you!

@chrisarevalodev
Copy link
Collaborator

Hey, @SarveshLimaye! Reviewed your changes and is looking very well. I just have two comments.

  1. For edge case please find a way to just print one error message instead of two for the same reason
    image
  2. I noticed that there's no indicator for which network are the tx's being queried from. So please add a console.log saying something like: Fetching transactions on Rootstock Mainnet or testnet, whatever the user chooses.
  3. Not sure if the API makes it possible but it would be cool to show the time the tx was made.
  4. Also it would be cool to have an optional parameter so the user can specify how many tx's wants returned (last 3, last 5, etc.)

@SarveshLimaye
Copy link
Author

SarveshLimaye commented Dec 18, 2024

Hey @chrisarevalodev, Thank you so much for reviewing the changes . I have addressed all four of your suggestions

  • Fixed edge case bug showing the same error twice
  • Added an indicator for which the tx's are being queried
  • Now the time the tx was made is also visible
  • Added an optional parameter so to specify how many tx's to be returned

It would be great if you could review the changes. I would love to hear any further suggestions.

Screenshot 2024-12-18 at 11 39 52 PM

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.

3 participants