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

Deprecate wallet2 and replace it with the Seraphis lib #3

Closed
j-berman opened this issue Apr 17, 2024 · 3 comments
Closed

Deprecate wallet2 and replace it with the Seraphis lib #3

j-berman opened this issue Apr 17, 2024 · 3 comments

Comments

@j-berman
Copy link

Goals

  • Unload the technical debt of wallet2 and replace it with a cleaner structured wallet using the Seraphis lib internally.
  • Don't lose the existing features of the GUI/CLI/RPC wallets.
  • Do this in the most efficient manner possible.

Challenges

  • wallet2 does the heavy lifting for wallet logic, so replacing all its functionality is going to take a lot of work.
    • Manages wallet state, keys, and settings.
    • Handles wallet scanning, tx construction, signing messages, constructing proofs, and more.
  • The CLI and RPC wallets are tightly woven with the wallet2 object (see m_wallet in simplewallet.cpp and wallet_rpc_server.cpp).
  • The GUI uses the wallet API, and the wallet API is tightly woven with the wallet2 object (see m_wallet in wallet/api/wallet.cpp).

Proposed plan

Step 1: wallet API

Stop using the wallet2 object directly in the wallet API, and instead use the Seraphis lib, except to construct legacy txs.

  • Benefits:
    • Takes a heavy load off reliance on wallet2 to manage state, keys, settings, and scanning.
    • Establishes a clean path to deprecate wallet2 once we no longer need to construct legacy txs.
    • Maintains the wallet API's existing functionality that wallet devs currently rely on.
    • Wallet devs that currently use the wallet API can start using the Seraphis lib sooner rather than later without needing to overhaul their wallets.
  • Challenges:
    • The wallet API is tightly woven with the wallet2 object (references m_wallet 193 times).
    • Will still need a "house" for wallet state and settings.
    • Will still need to be able to read a wallet2 keys file and wallet cache and migrate it over to a new file structure (related to 1 and 2).
    • Needs care to avoid turning the wallet API into another wallet2.
  • Estimated full-time work to complete: 3-6 months.

Step 2: CLI and RPC wallets

Stop using the wallet2 object directly in the CLI and RPC wallets, and instead use the wallet API.

  • Benefits:
    • Cuts off major dependencies on wallet2.
    • Minimizes time and maintenance burden to develop features for all of the GUI/CLI/RPC wallets in the future.
    • Enables reusing the suite of functional tests for the RPC wallet for the Seraphis lib.
    • Charts a path for wallets like Feather that also use the wallet2 object directly to use the wallet API exclusively.
  • Challenges:
    • Both CLI and RPC wallets are tightly woven with the wallet2 object (CLI references m_wallet 724 times, RPC 368 times).
    • Will likely need to extend functionality of the wallet API to allow for some of the behavior that the CLI and RPC wallets need.
      • e.g. the wallet API doesn't support wipeable strings when passing secrets around, which the CLI relies on to keep sensitive key material out of RAM. On this note, the wallet API actually keeps the wallet password in memory too which needs to change as @tobtoht proposed here.
  • Estimated full-time work to complete: 2 months.
    • There is a lot of logic in CLI and RPC that the wallet API duplicates, so it should mostly be a matter of replacing high level calls, rather than deep changes.

Note: technically the above 2 steps can be done in parallel, but I figured it would be a cleaner process to prioritize 1 first.

Why keep the wallet API?

  • Significantly reduces time to develop functional GUI/CLI/RPC wallets that maintain existing functionality.
  • Makes it significantly easier for wallet devs who currently use the wallet API to migrate to Seraphis.
  • Gets the Seraphis lib used by devs sooner rather than later.
    • Helps squash bugs in the Seraphis lib.
    • Gets faster scanning into user's hands sooner.
  • The wallet API is ok, not perfect, but ok. Dropping wallet2 would be a major win.
@rbrunner7
Copy link
Member

A first short comment: I had problems to find this issue again because the place is unusual. We have most issues over at https://github.com/seraphis-migration/wallet3/issues.

Maybe it wasn't the best move back then to open several different projects under that Seraphis migration org ...

@rbrunner7
Copy link
Member

Thinking about it a bit: Would you be ok to close this here, and re-open it over there?

@j-berman
Copy link
Author

Closed in favor of seraphis-migration/wallet3#64

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

No branches or pull requests

2 participants