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

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

Open
rbrunner7 opened this issue Oct 27, 2023 · 3 comments

Comments

@rbrunner7
Copy link
Member

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
Copy link
Member Author

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
Copy link

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
Copy link

DangerousFreedom1984 commented Oct 28, 2023

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.

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

3 participants