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

Port rusk-wallet to new wallet-core #2219

Merged
merged 2 commits into from
Sep 5, 2024
Merged

Port rusk-wallet to new wallet-core #2219

merged 2 commits into from
Sep 5, 2024

Conversation

Daksh14
Copy link
Contributor

@Daksh14 Daksh14 commented Aug 28, 2024 •
edited
Loading

  • Port rusk-wallet to new wallet-core
  • Add missing features in wallet-core for working with input notes

closes #2143

@Daksh14 Daksh14 marked this pull request as draft August 28, 2024 17:33
@Daksh14 Daksh14 marked this pull request as ready for review August 29, 2024 18:36
@HDauven
Copy link
Member

HDauven commented Aug 29, 2024

Awesome work so far. I've been testing it a bit, and the transfer seems to work as expected.

I've noticed the following bugs:

  1. When checking the transaction history right after sending a transaction the cache will panic with a Note must exists to be moved error.
  2. The default wallet with a local node will report there is no stake key when checking for stake info. But when I try to stake with it, it panics saying Can't stake twice for the same key.
  3. When trying to claim rewards from a wallet that should have rewards it panics with the following error: "Value withdrawn different from available reward"

I've sent you the details in DM @Daksh14. If you want me to file bug reports, please let me know!

@herr-seppia
Copy link
Member

herr-seppia commented Aug 30, 2024 •
edited
Loading

When trying to claim rewards from a wallet that should have rewards it panics with the following error: Value withdrawn different from available reward

This is currently an issue with our stake contract implementation.
Probably we can solve it allowing partial rewards

Can you open an issue?

/cc @ureeves

moCello
moCello requested changes Aug 30, 2024
View reviewed changes
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

Nice work! Apart from the rfc's, I have some general requests:

  • in "rusk-wallet/src/bin/command.rs" and "rusk-wallet/src/wallet/gas.rs" (and maybe other places too) why did you change Lux to u64? I think it's important to make it clear where a u64 is referring to units of Dusk and where it is referring to units of Lux. Both can be represented in u64 but 1 Lux is not the same as 1 Dusk
  • we now have Dusk implemented twice, once in execution-core and here in the currency module. I would like it to be in one place only and I think it makes the most sense to introduce the currency module in execution-core.
  • please remove all references to the spent-keys, in both the code and comments. The key naming changed and the spent-keys are now phoenix-keys. That means that the short psk, formerly referring to public-spend-key, now will likely be interpreted as phoenix-secret-key, causing confusion.
  • please refer to the crate-module as crate instead of dusk_wallet
  • Atm the workspace member has a different name than the crate itself but it might be nice to have them match and settle for both to either be dusk-wallet or rusk-wallet
  • I don't find the mechanism of how we delegate the proof-generation
  • the seed and all secret-keys need to be zeroized after being used

rusk-wallet/src/bin/interactive.rs Outdated Show resolved Hide resolved
rusk-wallet/src/cache.rs Show resolved Hide resolved
rusk-wallet/src/cache.rs Show resolved Hide resolved
rusk-wallet/src/clients/sync.rs Outdated Show resolved Hide resolved
wallet-core/src/lib.rs Show resolved Hide resolved
rusk-wallet/src/clients.rs Show resolved Hide resolved
rusk-wallet/src/wallet.rs Outdated Show resolved Hide resolved
rusk-wallet/src/wallet.rs Outdated Show resolved Hide resolved
rusk-wallet/src/wallet.rs Outdated Show resolved Hide resolved
rusk-wallet/src/wallet.rs Outdated Show resolved Hide resolved
herr-seppia
herr-seppia requested changes Aug 31, 2024
View reviewed changes
rusk-wallet/src/clients.rs Outdated Show resolved Hide resolved
rusk-wallet/src/clients.rs Show resolved Hide resolved
@Daksh14 Daksh14 force-pushed the rusk-wallet-core-update branch 6 times, most recently from 69029c0 to 5778e64 Compare September 4, 2024 13:58
@Daksh14 Daksh14 requested review from ureeves and removed request for ZER0 September 4, 2024 16:13
moCello
moCello requested changes Sep 4, 2024
View reviewed changes
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

Still some open RFC's

wallet-core/Cargo.toml Outdated Show resolved Hide resolved
rusk-wallet/src/error.rs Outdated Show resolved Hide resolved
rusk-wallet/src/store.rs Outdated Show resolved Hide resolved
rusk-wallet/src/store.rs Outdated Show resolved Hide resolved
wallet-core/src/lib.rs Show resolved Hide resolved
rusk-wallet/src/wallet.rs Outdated Show resolved Hide resolved
rusk-wallet/src/wallet.rs Outdated Show resolved Hide resolved
rusk-wallet/src/wallet.rs Outdated Show resolved Hide resolved
rusk-wallet/src/wallet.rs Outdated Show resolved Hide resolved
rusk-wallet/src/wallet.rs Outdated Show resolved Hide resolved
@Daksh14 Daksh14 force-pushed the rusk-wallet-core-update branch 7 times, most recently from 0abb72d to 0f383bd Compare September 5, 2024 02:03
moCello
moCello previously approved these changes Sep 5, 2024
View reviewed changes
Copy link
Member

@moCello moCello left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There are still some open review comments but I'll approve for now so that we can deal with them in follow up issues.

Please have another look at the commits though. After the first line there always needs to be an empty line.

@Daksh14 Daksh14 force-pushed the rusk-wallet-core-update branch 3 times, most recently from 00df9aa to 5599459 Compare September 5, 2024 14:59
@Daksh14
rusk-wallet: Upgrade to new wallet-core
a1e2e9d 
- Change crate name to
- Acclimate to the new naming scheme of keys and types
- Zerorize secret keys
- Acclimate to new phoenix_balance method
@Daksh14
wallet-core: Add input module
645ea51 
- Add algorithm for picking notes
- Re-export some commonly used types
@Daksh14 Daksh14 merged commit 8fa8ecb into master Sep 5, 2024
15 checks passed
@Daksh14 Daksh14 deleted the rusk-wallet-core-update branch September 5, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@herr-seppia herr-seppia herr-seppia approved these changes

@moCello moCello moCello left review comments

@HDauven HDauven Awaiting requested review from HDauven

@ureeves ureeves Awaiting requested review from ureeves

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

Successfully merging this pull request may close these issues.

rusk-wallet: Add wallet feature to Rusk
4 participants
@Daksh14 @HDauven @herr-seppia @moCello