Refactoring for more modularity

[ochaloup]

A "kick-off" refactoring that should bring better tree shakability in way of splitting the monolith class(es) to multiple method calls. This is a draft that is considered that need a discussion. The base code is drafted around marinade.ts but the code does not build smoothly, tests are not aligned, README misses to reflect changes...

As going through the refactoring cycles I was trying to have not so many arguments on the functions params but lastly I ended up with "asking" for all as method params. I was struggling with thinking if having MarinadeConfig class is beneficial (all config data placed at one class that could be passed through the application or if it's rather confusing). T he current version mostly remove the usage of it and uses only default values on input params.
The state should be managed by the caller as a base idea and just passed in. This is something I haven't elaborated through the whole code so depending on the feedback of this PR I will recheck that later.
I would be happy to learn some nicer syntax approaches or whatever that could be enhanced.

A question is if the API in the current for is easy to use by callers. As I do not work at FE side I'm not sure here. Maybe some wrapping of arguments of facading method calls could be easier to use but on the other way the tree shakability could be harmed.

Please, let me know what you think.

/cc @MjCage

[AlexStefan]

@ochaloup I really like the direction that this is taking! nice job! 🔥
I left two comments but did not dive too deep into the changes.

[ochaloup]

@AlexStefan I updated the PR, processed your review points (I hope) and fixed the tests to get running. This is ready for next round of the review.

Noticable changes:

• using direct imports from solana/web3js
• not re-exporting web3 from anchor (59510cd#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80L9)
• started to use MarinadeProgram class (59510cd) instead of using builders as it was in prior version. I considered to use a class that manages the program and state of marinade program and/or marinade referral program much easier and straightforward to use. It's one class that couples some dependencies but on the other side it seems to me still reasonable against the need to think at every call about programs, states and how to build the transaction (that "trouble" comes from fact that the Marinade is capable to manage calls either to liquid-staking-program or to liquid-staking-referral-program. If the design should split that to two separate call chains then it could be cleaned. On the other hand I assume it's seems ok the decisions being coupled within one implementation. Not sure though.
  Please take a look at the examples of the calls in the tests how the API calls ware changed.
• MarinadeConfig was completely removed as not need in calls. The file now contains the default parameters that are and can be used elsewhere.
• from the original (current) API the getter methods returns not only the data about the account but additionally there are added the pubkey and programId for the account (59510cd#diff-b7746310c0c1d56c09e39a660adb9acc70dc3ddfe2d1c0548dfdcdff6ac505a5R88). It seemed useful to me.
• I removed the object like function parameters (not sure about the TS term here like function foo({something}:{something: string})) and used just the parameters. But I intentionally left the object like function parameters at instruction builders. It seems that there should be those as there are multiple pubkeys with different names and it's really easy to make mistake in changing the order. With object like params is much easier to use default params as well. I hope it could be ok for you but we can change for sure.

What's missing:

• updating the js doc to reflect the changes. I would do that as finalization step when agreed on the shape of the code.

[AlexStefan]

We're on the right path! Would be cool if you could check the commits what I've pushed and the remaining comments. Afterwards I'll give it a try with the Marinade dApp to see if we broke anything or how hard the port would be.

[AlexStefan]

should we convert this into an assert? like the one at line 59?

[ochaloup]

I'm not sure what you mean here about assert to be used here.
Do you mean like assert(voteRecord, ErrorMessage.NO_PUBLIC_KEY).

That's not possible currently as how SDK is designed (as far as I understand it). When the voteRecord is not provided to the method then the caller does not expect an error. It's a normal flow of code.
When the instruction is not returned back to the caller the caller behaves differently.
It's not much intuitive I assume. Maybe the way how SDK API is designed here should be adjusted to be clearer.
WDYT?

The caller should behave differently when he provides no pubkey. The original code of SDK is not mine but it's from MJ so I just trying to understand the behaviour and adapt it.

[AlexStefan]

we need to update params for all the methods as this was changed

[ochaloup]

+1, updated. check if appropriately

[AlexStefan]

Food for thought: thinking of making this more generic and not repeating the same thing under different names, any opinions? 🤔

[ochaloup]

Can you please elaborate on what are the places where the same thing is repeated?

Maybe you refer to usage of the interface and similar naming of the methods? If so, then this patter was intentional from me as I wanted to have the same way to call the instruction builder for program and referral program and the interface made possible to join those two under uniform call. But a) maybe you refer to something else, b) maybe you have some different way to solve this or desing the sdk.

[ochaloup]

hi @AlexStefan,

• I merged with the main and fix tests.
• I responded to the non-resolved review items and did some fixes. We can talk about them here further.

For the changes you provided I have a bit uncertainty about that change from provider to connection.
a0c5d46#diff-2ef13923a0b594c47d0308a037a985b7a0521e71a7791be5a7326af3cb1f6579L60

I think I understand the point that it seems to me being to make the SDK simplier (more straightforward). What's an issue is the fact that the program is provided out of the SDK. Anybody can use it to take the provider and do the call and it won't be anyhow clear that this program is not capable to do that stuff and it's meant to be used only as a transaction builder. I mean MarinadeProgram.program.methods.addLiquidity(..).accounts({...}).rpc() will just fail as signature is not possible.
I like the thing that the provider is not really needed to be fully capable in the marinade sdk but for that would not be mis-used it should be hidden for outside usage.
WDYT about such a change? I haven't tested it if it makes some trouble for other parts of the SDK but to me for moving to connection only making provider and program private sound reasonably needed to me.