seraphis-migration / wallet3

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

Do we use the Seraphis library versioning of class names for our wallet classes as well? #62

Open rbrunner7 opened 1 year ago

rbrunner7 commented 1 year ago

If you look over typical Seraphis library code like in this file, it probably won't take long until you notice all the versioned struct and class names, like SpCoinbaseEnoteV1, SpMembershipProofV1 and so on.

The idea: Chances are quite high that Seraphis and Jamtis will develop further, and then the code will have to deal with several versions of things like enotes, transactions and so on, and do so concurrently, i.e. you cannot just change your SpEnoteV1. Instead of making the Seraphis enote class more flexible internally with if and switch statements, it may be better software engineering to put a new class SpEnoteV2 alongside SpEnoteV1 and then treat it as a separate problem to deal with a mixed bunch of objects of both classes somehow.

In this file, by the way, you already have versions 1 up to 5, because there are 5 different types of "legacy" enotes from the point of view of the Seraphis library.

After all this introduction and explaining, finally the question why I write this issue: Shall we do the same for most structs and classes of the Seraphis wallet code? @DangerousFreedom1984 recently made a PR containing a class to manage transactions and called it SpTransactionStoreV1. Do we support this as "best practice", or do we see it as "too much" and go here for simply SpTransactionStore?

rbrunner7 commented 1 year ago

My personal opinion, after thinking about it quite a bit: For our wallet classes I see this as overkill that I would like to rather avoid.

Of course they may grow in complexity over time, but I see one important element probably missing that it present in the Seraphis library: The need to support multiple variants concurrently.

If our transaction store needs to learn more and more tricks over time, because other components want to do queries in ever different ways, I don't see something stopping us to make "the" one transaction store more flexible. I don't see how that could sensibly lead to two classes SpTransactionStoreV1 and SpTransactionStoreV2 both being present in code, and both going forward.

jeffro256 commented 1 year ago

The need to support multiple variants concurrently.

This is probably the correct distinction to make. We should using versioned class names for talking about objects which we use to interact with other people and machines, which matter exactly which form they take. For example, SpEnoteV1 is an object which represents a concept/collection of data on the blockchain, whereas SpTransactionStore is a local manager/helper/tool class, of which the versioning doesn't really matter, as long as it works.

DangerousFreedom1984 commented 1 year ago

Good point. In this specific case if the TransactionRecord needs to handle JamtisPaymentProposalSelfSendV2 or V3 then it would be better to manage it using a switch indeed.