quark-proxy subproject #125
Conversation
fluffywaffles
force-pushed
the
jordan/direct-proxy-subproject
branch
2 times, most recently
from
e6e258d to
280b620
Compare
fluffywaffles
force-pushed
the
jordan/direct-proxy-subproject
branch
from
280b620 to
f3c551e
Compare
quark-core/src/QuarkScript.sol
Outdated
| function signer() internal view returns (address) { | ||
| (bool success, bytes memory signer_) = address(this).staticcall(abi.encodeWithSignature("signer()")); | ||
| if (!success) revert("no signer"); | ||
| return abi.decode(signer_, (address)); | ||
| } |
There was a problem hiding this comment.
ideally these helpers in QuarkScript.sol should get inlined when called.
Moreover, I think we can probably remove the success variable and if (!success) code, seeing as if this call fails, the wallet is invalid anyway.
quark-core/src/QuarkWallet.sol
Outdated
| /** | ||
| * @dev Internal getter for the signer immutable | ||
| */ | ||
| function getSigner() internal view returns (address) { | ||
| (bool success, bytes memory signer_) = address(this).staticcall(abi.encodeWithSignature("signer()")); | ||
| if (!success) revert("no signer"); | ||
| return abi.decode(signer_, (address)); | ||
| } | ||
|
|
||
| /** | ||
| * @dev Internal getter for the executor immutable | ||
| */ | ||
| function getExecutor() internal view returns (address) { | ||
| (bool success, bytes memory executor_) = address(this).staticcall(abi.encodeWithSignature("executor()")); | ||
| if (!success) revert("no executor"); | ||
| return abi.decode(executor_, (address)); | ||
| } |
There was a problem hiding this comment.
Similarly to QuarkScript.sol, in QuarkWallet.sol I think we can probably remove the success variables and if (!success) cases in the getters, seeing as if the calls fail, the wallet is invalid anyway.
| /** | ||
| * @dev Internal getter for the signer immutable | ||
| */ | ||
| function getSigner() internal view returns (address) { |
There was a problem hiding this comment.
I would rename this method signer() and getExecutor() to executor(), but to be backwards-compatible with a wallet deployed without a proxy, we cannot re-use these names.
ideally, the staticcalls should be inlined, anyway. hopefully, as internals, they will be -- but I am also considering moving them into the QuarkWalletMetadata library where I am fairly certain they'll be inlined.
There was a problem hiding this comment.
I think internal functions use a JUMP instead of being inlined. This is how we cut down on bytecode size sometimes, by moving inline code used in multiple places into a shared internal function. I think this is fine for now, we can optimize later.
| /// @notice Address of the QuarkWallet implementation contract | ||
| address public immutable walletImplementation; |
There was a problem hiding this comment.
I believe we can actually predict a valid wallet implementation address for use by the proxy: the "null wallet," or QuarkWallet{salt: 0}(address(0), address(0)). It can never execute code itself, but if it is delegatecalled its implementation can run in the context of the proxy.
The benefit here is that the address is deterministically predictable, which means we could replace this immutable with a constant, which would save on gas costs for both deployment and calls, since the constant will by definition be inlined by the compiler (and we can remove a line of code from the constructor).
There was a problem hiding this comment.
It's only deterministic if deployed via CodeJar, but CodeJar doesn't support contracts with constructors right?
| function testSignerExecutor() public { | ||
| assertEq(walletImplementation.signer(), address(0)); | ||
| assertEq(walletImplementation.executor(), address(0)); | ||
|
|
||
| assertEq(aliceWalletProxy.signer(), aliceAccount); | ||
| assertEq(aliceWalletProxy.executor(), address(0xabc)); | ||
|
|
||
| bytes memory testScript = new YulHelper().getDeployed("ProxyDirect.t.sol/TestHarness.json"); | ||
| QuarkWallet.QuarkOperation memory op = new QuarkOperationHelper().newBasicOpWithCalldata( | ||
| QuarkWallet(payable(aliceWalletProxy)), | ||
| testScript, | ||
| abi.encodeWithSignature("getSignerAndExecutor()"), | ||
| ScriptType.ScriptAddress | ||
| ); | ||
| (uint8 v, bytes32 r, bytes32 s) = | ||
| new SignatureHelper().signOp(alicePrivateKey, QuarkWallet(payable(aliceWalletProxy)), op); | ||
| bytes memory result = QuarkWallet(payable(aliceWalletProxy)).executeQuarkOperation(op, v, r, s); | ||
| (address signer, address executor) = abi.decode(result, (address, address)); | ||
| assertEq(signer, aliceAccount); | ||
| assertEq(executor, address(0xabc)); | ||
| } | ||
| } |
There was a problem hiding this comment.
This test demonstrates that the null wallet can be used as an implementation wallet for the proxy.
fluffywaffles
force-pushed
the
jordan/direct-proxy-subproject
branch
from
a0ac662 to
656a685
Compare
| /** | ||
| * @dev Internal getter for the signer immutable | ||
| */ | ||
| function getSigner() internal view returns (address) { |
There was a problem hiding this comment.
I think internal functions use a JUMP instead of being inlined. This is how we cut down on bytecode size sometimes, by moving inline code used in multiple places into a shared internal function. I think this is fine for now, we can optimize later.
| /// @notice Address of the QuarkWallet implementation contract | ||
| address public immutable walletImplementation; |
There was a problem hiding this comment.
It's only deterministic if deployed via CodeJar, but CodeJar doesn't support contracts with constructors right?
| assertEq(aliceWalletProxy.executor(), address(0xabc)); | ||
|
|
||
| bytes memory testScript = new YulHelper().getDeployed("ProxyDirect.t.sol/TestHarness.json"); | ||
| QuarkWallet.QuarkOperation memory op = new QuarkOperationHelper().newBasicOpWithCalldata( |
There was a problem hiding this comment.
nit: pause gas metering for this
fluffywaffles
force-pushed
the
jordan/direct-proxy-subproject
branch
from
f428572 to
ca46b29
Compare
|
had to rebase with |
No description provided.