search

seraphis-migration / wallet3

Info and discussions about a hypothetical full 'wallet2' rewrite from scratch
MIT License
14 stars 0 forks source link

Deprecate wallet2 and replace it with the Seraphis lib #64

Open j-berman opened 7 months ago

j-berman commented 7 months ago

Goals

Challenges

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.

Step 2: CLI and RPC wallets

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

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?

j-berman commented 7 months ago

@r4v3r23 proposed an interesting idea as well to me in DM's.

Before replacing the entirety of wallet2 in the wallet API in Step 1, Step 0.5 could be to replace just the scanner. My knee-jerk reaction was it might cause more headache later, but it should be smooth if done like this pseudocode:

wallet_api::refresh()
    seraphis_lib::refresh_enote_store()
    populate_wallet2_instance_from_seraphis_enote_store()

Since we'd want to be able to use wallet2 "in a box" to construct a tx, then we'd need populate_wallet2_instance_from_seraphis_enote_store to work correctly anyway. Starting with the scanner like this could be a solid incremental step. That's already in the thousands of lines of wallet2 code replaced by the Seraphis lib.

rbrunner7 commented 7 months ago

Thanks for moving this, appreciated.

Interesting proposal, and I guess an almost logical consequence of all now quite substantial "delays" that we have with implementing our new wallet, for various reasons: The ideal, a brand-new API designed out on a green field, is probably a luxury we can't afford anymore timewise.

A question: Why take wallet_api as base for the whole endeavor, and not directly wallet2.h itself? Does wallet_api just add the right amount of abstraction on top of that to make the switching of more or less all code underneath feasible in the first place? Strikes me as a bit improbable, but I don't know that API in detail.

If we can pull off your trick even with wallet2.h itself, compatibility could be even better, no?

j-berman commented 7 months ago

I don't see much benefit to getting rid of wallet2.cpp going that route. I figure if we were to keep wallet2.h, we might as well just extend wallet2.cpp for Seraphis reusing individual components from the Seraphis lib.

I do think the wallet API offers a solid base of abstraction to build from that makes it easier to drop wallet2.h and wallet2.cpp in favor of a cleaner wallet structure.

This is generally postulation on my part though from eyeing the code and working with it.

I'm strongly warming to this idea to start an incremental approach with the scanner.

rbrunner7 commented 7 months ago

Hmm, you are not afraid that if you add absolutely everything that wallet apps need for their fully current functionality, wallet_api starts to look a lot like wallet2.h?

j-berman commented 6 months ago

Ya it's a concern, I included that as a challenge to this approach. I think following similar coding practices as the Seraphis lib (clean class separation, shorter functions, measured design decisions to name a few), it's avoidable.

Example: the Seraphis lib's modular scanner shows how we can replace a major component of wallet2 with cleaner structured code. That's why I'm thinking the scanner is a good place to start.

r4v3r23 commented 6 months ago

i support this modular approach

since according to rbrunner7 a new api is out of the question, reusing the current one and making the seraphis essentially a drop-in replacement update for current wallets is a massive plus

will be helping @j-berman test his upcoming scanner once its ready

DangerousFreedom1984 commented 6 months ago

I agree that now we have so many Seraphis components that integrating into the existing code starts to be the main challenge. I like your organizational proposal as it tries to set a clear path to use some seraphis features though I dont know if it is the best approach to build wallet3 since we cant add all features into the wallet2_api and it constrains us with wallet2 design. But at least looks like we have an approach now. So let me rephrase some of your ideas to try to understand it and let me try to frame what I can do to contribute.

Also, I disagree with @rbrunner7 here. Looking at how the seraphis_lib work, Koe did such an excellent job that it is already doing a great deal of the work that the wallet2 functions do. Therefore implementing a wallet and an api for seraphis should be much easier than implementing wallet2 again.

Interesting proposal, and I guess an almost logical consequence of all now quite substantial "delays" that we have with implementing our new wallet, for various reasons: The ideal, a brand-new API designed out on a green field, is probably a luxury we can't afford anymore timewise.

Anyway, moving forward with this thought.

Step 1: wallet API - stop using wallet2 (m_wallet) but keep using the API

How exactly would we do that?

I can imagine taking the following path:

We create a wallet3.h(.cpp) under src/wallet/api/ to implement the wallet2_api.h interface.

So it means implementing the methods from: WalletListener, Wallet, WalletManager and so on.

wallet3 here would call the src/seraphis_wallet/* functions.

Is that what you think?

I think it would be nice to initially target a high degree of compatibility with the GUI therefore using the wallet2_api.h as the interface would be a good first approach in that sense. I believe if that is achieved, all the other wallets wouldnt have big troubles to migrate (given that the interface is very similar). But of course it can't be achieved 100% since jamtis_seraphis have many new features that are not contemplated in that interface. But adding new features could be done incrementally and the old methods could be replaced with the new wallet3 methods progressively (as well as the style since we would be migrating from camelCase to snake_case :p).

In the same way, since the CLI uses wallet2 directly. We could also target creating a new CLI that uses the wallet2_api.h interface initially and then migrate to wallet3_api.h to fully support jamtis_seraphis if/when needed. I believe I could target that in my CCS (basically I'm trying to have the basic functions of a wallet using the PoC CLI that I created and with the help of @jeffro256 we hopefully will be able to create demonstrator that is integrated with the LMDB database.) so in the end we would have a demonstrator of a CLI wallet3 that uses the wallet2_api.h / wallet3_api.h that does the basic functions of the wallet. What do you think?

j-berman commented 6 months ago

I was thinking something a bit different.

How exactly would we [stop using the wallet2 object]?

I was imagining systematically going through each m_wallet-> function and replacing with Seraphis lib-style structured code.

I loosely categorize what m_wallet is used for in src/wallet/api/wallet.cpp as follows:

I think most functions could be structured better and "housed" into a better bin. I was thinking something along these lines...

The keys can get their own class referenced via m_wallet_keys which has its own readers and modifiers.

The settings can get their own class referenced via m_wallet_settings which has its own readers and modifiers.

The enote store can capture wallet state. We'd rewrite wallet2 functions into Seraphis-style functions (refresh_enote_store being a perfect example as replacement for m_wallet->refresh) to read/mutate/use wallet state.

Static functions can be refactored.

DangerousFreedom1984 commented 6 months ago

Interesting. So basically the bug free seraphis_wallet is the non existing seraphis_wallet ? :p Basically you would 1) refactor wallet2 and 2) add new functionalities to it, right?

j-berman commented 6 months ago

Would eliminate dependencies on wallet2.cpp and wallet2.h, and add new functionalities to src/wallet/api/{wallet.cpp, wallet.h, wallet2_api.h, wallet_manager.cpp, wallet_manager.h} as needed to replace lost functionality

rbrunner7 commented 6 months ago

I have been reading through the discussion so far and tried to understand the various ideas and approaches mentioned here, and I am not sure whether I fully understood what everbody tried to say and whether also people fully understood each other so far. This is perhaps not surprising, because it's a difficult endeavor.

Faced with a difficult and complex programming task I always try to radically simplify it first and find a way to do it using steps of manageable size. IMHO in this case, all on my own, I would probably try the following first:

Put away any Seraphis library code for the time being. Work strictly only within the frame of existing Monero code. Basically, just complete at long last what that "Wallet API" originally set out to do but did not fully accomplish: Create a new, cleaner and better structured wallet API that supersedes wallet2.h.

End goal: You can write any wallets and wallet-like apps like the RPC server, even fully featured ones like the CLI wallet, by using the "Wallet API" only, without using anything from wallet2.h directly.

That would mean going systematically going through wallet2.h and the "Wallet API" header files in parallel, make a "gap analysis" i.e. identify what functionality of wallet2 is not yet available through the "Wallet API", add carefully designed new classes, methods, structs, enums etc. for that missing functionality, and then implement those, based on wallet2.h, possibly other existing lower Monero classes, or new code.

Hopefully the amount of new code needed would be pretty small, with most code just use one thing or another from wallet2.h to do the "heavy lifting" - just with a better API, better structure, and better naming.

Result would be something that we could PR to the existing Monero codebase without problems, because, again, it would not yet include any parts of the Seraphis lib, reviews should be possible in a short timeframe, with many people being able to review in the first place, and wallet devs could start to use it anytime.

Of course, once that's out of the door, you can continue and almost immediately start to use e.g. @j-berman 's new scanner together with the Seraphis lib parts that it needs.

SNeedlewoods commented 6 months ago

End goal: You can write any wallets and wallet-like apps like the RPC server, even fully featured ones like the CLI wallet, by using the "Wallet API" only, without using anything from wallet2.h directly.

FWIW no one disagreed with the end goal.

That would mean going systematically going through wallet2.h and the "Wallet API" header files in parallel, make a "gap analysis" i.e. identify what functionality of wallet2 is not yet available through the "Wallet API" ...

I would even break it down further and add an intermediate goal: Get rid of all wallet2.h code in api/wallet.h and replace it, without adding new functionality first.

Here is an incomplete list of "API Tasks" that I see so far:

  1. Get rid of wallet2.h in api/wallet.h
    • Seperate changes into new classes/files if suitable:
    • WalletSettings
    • WalletKeys
    • WalletState* (like EnoteStore)
    • Maybe add a Blockchain class, which would replace functionality from hashchain class in wallet2.h.
    • api/utils.cpp (imo some functions don't need to directly be part of WalletImpl)
    • e.g. uint64_t wallet2::estimate_blockchain_height()
    • Serialization should get seperated* (maybe into wallet_api_serialization.h following the design from jeffro256 here https://github.com/UkoeHB/monero/pull/39/files).
  2. Add all necessary features needed for RPC/CLI wallet.
  3. Add a layer for multisig (e.g. a class which inherits from WalletImpl from api/wallet.h), there shouldn't be any multisig code directly in api/wallet.h.*

* I'm not sure about this.

First I planned to handle WalletSettings only and make a PR, then make another PR for WalletKeys and so on. But after spending some time implementing things for WalletSettings I figured, how much everything is intertwined, so now I'm leaning towards "bite-sized" PRs where just a single function is targeted, but with the whole rattail (every change needed to make it work), I assume this would lay out a better foundation for later work.