Conversation
adamskrodzki
left a comment
adamskrodzki
left a comment
There was a problem hiding this comment.
I like general idea, but it can be refined
| const dsproxy_calldata = system.operationRunner.interface.encodeFunctionData("executeOperation", | ||
| [{ | ||
| name: 'openVaultOperation', | ||
| callData: [[openVaultCalldata],[openVaultCalldata]], |
| string name; | ||
| bytes[][] callData; | ||
| bytes32[] actionIds; | ||
| address serviceRegistryAddr; |
There was a problem hiding this comment.
serviceRegistryAddress can be immutable instead I guess it will be allways the same
| Operation memory operation, | ||
| uint256 _index, | ||
| bytes32[] memory returnValues | ||
| ) internal returns (bytes32 response) { |
There was a problem hiding this comment.
if we are about to support return values, i think it should be bytes not bytes32
| import "./ActionBase.sol"; | ||
| import "hardhat/console.sol"; | ||
|
|
||
| contract Deposit is ActionBase { |
There was a problem hiding this comment.
Is this a right name ? looks like open empty vault to me
|
|
||
| struct Operation { | ||
| string name; | ||
| bytes[][] callData; |
There was a problem hiding this comment.
why it is multidimentional? I'm quite sure we can get away with one dimention and then do abi.decode
similar trick like:
generally it is reasonable to assume that action knows format of its parameters, so You pass bytes to action and it decodes it in any way it wants
| return ""; | ||
| } | ||
|
|
||
| function isFlashLoanAction(address actionAddr) internal pure returns (bool) { |
There was a problem hiding this comment.
is it used after all?
| import "hardhat/console.sol"; | ||
| import "../ServiceRegistry.sol"; | ||
|
|
||
| struct Operation { |
There was a problem hiding this comment.
in 0.8+ solc and in 0.7 with abie encoder v2 pragma constructs like:
pragma solidity ^0.7.6;
pragma abicoder v2;
struct Action{
bytes32 id;
bytes data;
}
struct Operation{
Action[] actions;
}
contract Silly {
function execute(Operation calldata op) public returns (bool){
}
}
are possible
|
|
||
| abstract contract ActionBase { | ||
| enum ActionType { | ||
| FLASHLOAN, |
There was a problem hiding this comment.
I see what You tried to do with this,
I think there ate two kinds of operations
- with callbacks
- without callbacks
but maybe there is really only one, and fact that operaation makes a callback is its internal detail as long as valle address is passed as parameter
| import "./ActionBase.sol"; | ||
| import "hardhat/console.sol"; | ||
|
|
||
| contract Flashloan is ActionBase { |
There was a problem hiding this comment.
I guess this is just placeholder,
flashloan can be (I think ) just and Operation like any other, it takes some some parameters that describes flash loan and then another operation which it executes on a runner.
| gasPrice: 1000000000, | ||
| }) | ||
|
|
||
| const lastCDP = await getLastCDP(provider, signer, system.userProxyAddress) |
There was a problem hiding this comment.
add at least assertion for a vault owner, to prove, that context is used correctly
| @@ -0,0 +1,179 @@ | |||
| //SPDX-License-Identifier: Unlicense | |||
There was a problem hiding this comment.
try defaulting to external visibility where possible. see https://ethereum.stackexchange.com/questions/19380/external-vs-public-best-practices
There was a problem hiding this comment.
there is a pending PR on automation repo to change all ServiceRegistry visibility as well
rkrasiuk
left a comment
rkrasiuk
left a comment
There was a problem hiding this comment.
i like the general direction where this is going, will give additional comments when the PR is ready for review
3 participants
No description provided.