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

rusk-wallet: Implement moonlight history #3284

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

Conversation

Daksh14
Copy link
Contributor

@Daksh14 Daksh14 commented Dec 22, 2024

Closes #2271

Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

Would be possible to have a tx history containing both Phoenix and moonlight transactions?
Why did you split them?

Additionally (but not blockers):

  • try to store the history in rocksdb to improve performance and unload archival computation (we already store Notes, you only miss the derived TransactionHistory objects)
  • add a warning if the user is not connected to an archival

@Daksh14
Copy link
Contributor Author

Daksh14 commented Dec 23, 2024

Would be possible to have a tx history containing both Phoenix and moonlight transactions?
Why did you split them?

Additionally (but not blockers):

  • try to store the history in rocksdb to improve performance and unload archival computation (we already store Notes, you only miss the derived TransactionHistory objects)
  • add a warning if the user is not connected to an archival

Mixing the moonlight and phoenix tx history would be confusing no? I can add another column that displays transaction type but that would add more info and make it harder to read

I split the history because all other methods are also split on public/private it makes sense.

Can you create issue for those two points you mentioned?

@Neotamandua
Copy link
Member

Mixing the moonlight and phoenix tx history would be confusing no? I can add another column that displays transaction type but that would add more info and make it harder to read

In the long run, I think it is necessary to add such a column anyway. Because we want to differentiate between different types of transactions (transfer out, transfer in, conversion, deposit, withdrawal, etc.).

@herr-seppia
Copy link
Member

Because we want to differentiate between different types of transactions (transfer out, transfer in, conversion, deposit, withdrawal, etc.).

These infos are already available in the transaction history in the "method" column IIRC

@Daksh14
Copy link
Contributor Author

Daksh14 commented Dec 28, 2024

@herr-seppia I merged them both and made another PR for pagenation
@herr-seppia @Neotamandua

@Daksh14 Daksh14 requested a review from herr-seppia December 28, 2024 20:04
@Daksh14 Daksh14 force-pushed the moonlight-history branch 2 times, most recently from dd3f285 to 3f6349c Compare December 28, 2024 21:43
rusk-wallet/src/bin/command.rs Outdated Show resolved Hide resolved
rusk-wallet/src/bin/command.rs Outdated Show resolved Hide resolved
rusk-wallet/src/error.rs Outdated Show resolved Hide resolved
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.

rusk-wallet: Manage transaction history for Moonlight
3 participants