QuarkWalletProxyFactory and test suite

[fluffywaffles]

here's a summary side-by-side comparison from running forge test --match-contract Factory and adding some annotations that show the results of some simple math:

[2,023,885 savings] [PASS] testCreateAdditionalWalletWithSalt()        (gas: 2245719 vs 221834) (2245719 - 221834 = 2,023,885 savings)
[1,021,869 savings] [PASS] testCreateAndExecuteCreatesWallet()         (gas: 1393133 vs 371264) (1393133 - 371264 = 1,021,869 savings)
[995,353   savings] [PASS] testCreateAndExecuteSetsMsgSender()         (gas: 1218394 vs 223041) (1218394 - 223041 = 995,353 savings)
[1,091,563 savings] [PASS] testCreateAndExecuteWithSalt()              (gas: 1482648 vs 391085) (1482648 - 391085 = 1,091,563 savings)
[1,037,353 savings] [PASS] testCreateAndExecuteWithSaltSetsMsgSender() (gas: 1270262 vs 232909) (1270262 - 232909 = 1,037,353 savings)
[2,077,681 savings] [PASS] testCreatesWalletAtDeterministicAddress()   (gas: 2315989 vs 238308) (2315989 - 238308 = 2,077,681 savings)
[984,990   savings] [PASS] testDefaultWalletHasNoExecutor()            (gas: 1089645 vs 104655) (1089645 - 104655 = 984,990 savings)
[1,021,679 savings] [PASS] testExecuteOnExistingWallet()               (gas: 1391676 vs 369997) (1391676 - 369997 = 1,021,679 savings)

small diffs in such a simple test seem like a fluke / variability:
[PASS] testDefaultWalletIsSubwalletExecutor()      (gas: 320763  vs 324885) (320763 - 324885 = -4122 added cost [?])

reverts don't measure gas properly:
[PASS] testCreateRevertsOnRepeat()                 (gas: 8937393460516764221 vs 8937393460516733240) (8937393460516764221 - 8937393460516733240 = 30981 [?])

overall we're looking at 1,281,796 average gas savings, removing outliers.

average cost now seems to be 269,136 gas, as opposed to >1mn gas.

Those are the costs per test-case; the traced cost of the actual call to factory::create(..) ranges from 88,000 to 100,000 gas depending on optimization settings. 88,000 seems to be the optimal result.

This comes out to roughly $9 at an estimated cost-per-gas of 50 gwei with an Eth price of $2,000.

Deploys with salt are ~5-10k gas more expensive, due to added call overhead for computing the executor address.

The factory test suite is abstracted and implemented for both QuarkWalletFactory and QuarkWalletProxyFactory; if we commit to the proxy approach, we may choose to streamline things and get rid of the abstraction. Similar to how the factory implementations share an abstract base contract, themselves.

I'm inclined to keep both approaches, and to keep sharing the test suite, although the indirection is a little unusual. Bike-sheds on how to structure this to make it less confusing or weird are welcome. I'm also open to arguments that we should remove the non-proxy factory code entirely.

[cwang25]

nit: This look like regular operation. I think we should use test/lib/QuarkOperationHelper.sol as much as possible if we plan on using the helper, unless it's some special operation struct that helper can't create. (don't think having some use helper and some don't scattering across test cases :P )

[fluffywaffles]

Hmm, also copied. I guess it didn't use the helper. Will update.

[fluffywaffles]

yeah, none of the tests in this suite were ever refactored to use the helper. Will update them now.

[fluffywaffles]

actually, these tests cannot use the helper; the helper depends on the wallet already existing, so that it can use wallet.stateManager().nextNonce(address(wallet)), which it cannot do if the wallet is not yet createAndExecuted.

[kevincheng96]

Nice work 👍

It is good to have both versions of the factory in the repo for comparison's sake, but the extra inheritance does make it harder to read the contract. I'm slightly leaning towards removing the non-proxy factory just to make the contracts cleaner; but we can do that after we finish these gas comparisons.

Any way we can compare the execution costs of typical Quark operations? Would be good to quantify the execution overhead before we fully commit to the proxy approach.

[kevincheng96]

This can be put in the abstract test file right?

[fluffywaffles]

hmm, AbstractQuarkWalletFactory does not have a codeJar member. So not really.

I think having a codeJar member is an implementation detail. Strictly speaking, the factory does not directly depend upon either of codeJar or stateManager. You could image a proxy factory that only takes a walletImplementation to its constructor, and never references codeJar or stateManager at all.

[fluffywaffles]

Any way we can compare the execution costs of typical Quark operations? Would be good to quantify the execution overhead before we fully commit to the proxy approach.

Hmm, there are a couple of ways I think we could go about this. We could try to abstract the QuarkWallet test suite, so that we can run the same test suite with a proxy -- or we could just quantify the overhead by tracing some calls.

I wish it was possible to write a test suite that took its dependencies as arguments. It's annoying that you cannot.

[fluffywaffles]

Any way we can compare the execution costs of typical Quark operations? Would be good to quantify the execution overhead before we fully commit to the proxy approach.

Hmm, there are a couple of ways I think we could go about this. We could try to abstract the QuarkWallet test suite, so that we can run the same test suite with a proxy -- or we could just quantify the overhead by tracing some calls.

I wish it was possible to write a test suite that took its dependencies as arguments. It's annoying that you cannot.

This was done in #141 to analyze the gas overhead introduced by the proxy. We will probably not merge this PR, but may wish to clean it up in the future to apply the quark wallet test suite to the proxy.

[fluffywaffles]

closed out by #146