Morpho Vault integration (p3)

[cwang25]

• Morpho Vault integration (earn flow)

[fluffywaffles]

Overall, this looks straightforward enough, and the test coverage looks pretty good. I don't see any missing revert cases, that kind of thing. 👍

However, I don't entirely understand what the XXX comments are about withdrawal checks that we don't implement. What are the "test that you have enough of the asset"-type comments about?

I brought up in a previous PR in this series that I think we should either (a) actually assert against the EIP712 data blobs or (b) drop the TODO that says we should. I admit to not seeing a lot of value in these tests, but (as far as I can tell) the only thing preventing us from testing the EIP712 data is the tedium of copy/pasting the expected hashes and whatnot into the test-case, so we should either go ahead and do that... or decide not do it. We should not leave behind a lingering TODO.

As I look over this code, having just looked at #67, I am struck by how much repetition there is. I wonder if there is not a way to abstract over some of the duplicated code here? Maybe there is not, but it puts an itch in my brain that there are some 100s of lines of code that differ only in the tiniest ways that are copy-pasted between methods of QuarkBuilder.sol. For example, there are code blocks in both PRs for // Validate generated actions for affordability, and these code blocks are really similar-looking at first blush, and sort of long-winded and ugly. I wonder if the logic could be refactored into a helper.

[fluffywaffles]

Is this still a to-do?

[cwang25]

Yeah this is copied from the cometWithdraw builder, and since both are mostly the same, so I'm pretty sure if cometWithdraw still have this XXX morpho will have the same as well.

From my understanding this is an optional feature that it's left undecided from the beginning from cometWithdraw.
Which we make builder to verify the input and revert if client ever pass in a larger amount to withdraw while user position doesn't have that much of asset in chainAccountList.

But also we don't really need to check, as client won't likely to send in a request to withdraw larger amount than user has. Plus client also provide the ChainAccountList, so if something very wrong in client causing client thinks user has more to withdraw than user has, it's likely client may also mess up the ChainAccountList as well which builder won't be able to catch either.

[cwang25]

Overall, this looks straightforward enough, and the test coverage looks pretty good. I don't see any missing revert cases, that kind of thing. 👍

However, I don't entirely understand what the XXX comments are about withdrawal checks that we don't implement. What are the "test that you have enough of the asset"-type comments about?

I brought up in a previous PR in this series that I think we should either (a) actually assert against the EIP712 data blobs or (b) drop the TODO that says we should. I admit to not seeing a lot of value in these tests, but (as far as I can tell) the only thing preventing us from testing the EIP712 data is the tedium of copy/pasting the expected hashes and whatnot into the test-case, so we should either go ahead and do that... or decide not do it. We should not leave behind a lingering TODO.

As I look over this code, having just looked at #67, I am struck by how much repetition there is. I wonder if there is not a way to abstract over some of the duplicated code here? Maybe there is not, but it puts an itch in my brain that there are some 100s of lines of code that differ only in the tiniest ways that are copy-pasted between methods of QuarkBuilder.sol. For example, there are code blocks in both PRs for // Validate generated actions for affordability, and these code blocks are really similar-looking at first blush, and sort of long-winded and ugly. I wonder if the logic could be refactored into a helper.

Yeah I figured repetition is a bit of pain, might need to major refactor and see if we can have a way to further modularize the builder. (Unfortunately solidity doesn't allow struct inheritances, so we have multiple structs with similar fields :( ). But I'm having some ideas of implementing some modularize functions to reduce those repetitions. Probably will be my next item after morpho integration if no one has picked that one up yet by then.

[kevincheng96]

Overall looks great. There does seem to be some weirdness with the action contexts that needs to be addressed.

I also agree with Jordan that we should think more about modularizing the code. It is becoming difficult to review these changes because a lot of the logic is copy-and-paste with some small changes. It'll definitely give me a bit more confidence if the copy-and-paste logic can be shared so I know that the same code path is running for the different supply/withdraw actions on different lending protocols.

[fluffywaffles]

Notes are addressed, and we've decided to commit to some refactoring of QuarkBuilder as a follow-up while we integrate builder pack. So, Morpho should be unblocked! 👍