blockworks-foundation / mango-v3

Mango Markets V3 Smart Contract
MIT License
151 stars 90 forks source link

state of mango program, thoughts on refactoring/clean-up in 2022 #101

Open microwavedcola1 opened 2 years ago

microwavedcola1 commented 2 years ago

placeholder issue, will update with some thoughts later, some pain points I recently encountered

would be great if people could add their own suggestions, we can consolidate, prioritize and start chopping them off

mschneider commented 2 years ago

It’s probable that the value of anchor integrations could be unlocked by a simple cpi library that simplifies interacting with mango similar to how serum is integrated

dafyddd commented 2 years ago

I agree scrolling across all these lines is a bit cumbersome. At this point I just have to use ctrl f for everything. But one advantage of current setup is simplicity and logic. Every instruction has an equivalent function in processor.rs. A refactor that would continue to be simple and logical would be nice if you can think of one.

Do you know all the drawbacks of going to Anchor? It would be useful to even just see the difference in execution speed

Tests were started by another dev who didn't see it all the way through. I think we ultimately moved on to testing in devnet because it takes just as much time and seems more realistic. If you could clean up the tests we could start using it again. By clean up I mean removing unnecessary files and making sure all instructions are hit and it's relatively fast to run the tests.

ckamm commented 2 years ago

Something I just came across was that some function signatures involving AccountInfo (like pub fn init_vals(..., open_orders_ais: &[AccountInfo; MAX_PAIRS]) in HealthCache) make mango logic unnecessarily hard to reuse externally.

The reason is that AccountInfos contain mut references and can sometimes be impossible to create without extra copies. Refactoring to parse these accounts outside and then only feeding in the results would simplify that a lot. I'll probably do that directly since it makes my current project easier.

dafyddd commented 2 years ago

@ckamm Interesting point. I believe it's the way it is right now because a lot of those AccountInfo objects are actually of the zero key. The new way of passing these in is to use Option<&OpenOrders> or a Vec of the same. But older code required users to just pass in all the open orders accounts in a big array and leave zero key for the ones not in margin basket. Just something to keep in mind when refactoring.

microwavedcola1 commented 2 years ago

todo: cleanup branches which have been abandoned, not-relevant anymore

microwavedcola1 commented 2 years ago

todo: some of the instructions dont have a corresponding helper instruction, useful for programs composing on top of mango, double check, and add missing ones

microwavedcola1 commented 2 years ago

todo: something like anchor debug would be nice while developing tests, so that we dont have to manually print all the keys

microwavedcola1 commented 2 years ago

ckamm: another thing for a cleanup/refactoring: passing these as arguments is error prone. It'd be nice if ChangeSpotMarketParams was a struct that could just be passed onwards to the executor function

microwavedcola1 commented 2 years ago

dark green highlights are instructions which dont have a correponding test atm

image

microwavedcola1 commented 2 years ago

whatever we do should take into account timeline for next protocol re-write, could be potential waste of effort

microwavedcola1 commented 2 years ago

the problem with using one of the accounts not from parameters but from import confuses api users, unless one enables full debug logging its hard to know that the token program also has to be sent as a account info while cpi'ing image

microwavedcola1 commented 2 years ago

nice sdk like https://github.com/zetamarkets/fuze or spl-token sdk