Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MetaMorpho D3M #117
MetaMorpho D3M #117
Changes from 48 commits
d8f53b4
439f8ae
a967466
697f0c3
0b6de36
959cdc8
3d84643
66b01df
5e39eca
ac32c5a
ab9846e
062b526
a888da1
b1cb6a2
d09a093
057b0cb
de4730a
1e49d36
a0efe5d
3fef536
3c5006f
2643eb9
42c6a68
bc257f5
c9348c6
cb604b1
3052f83
ce81124
db98418
d4e2110
03af9eb
2d79650
9183aae
f65e1a0
686e304
c6e9214
5e655cc
c160121
3615bb4
bbdea8b
698ec5c
da5ae45
d8a85f8
efa9fd3
940cb93
da33537
6cab878
7f9752a
574965d
6b827d0
bcd532c
3a008e2
5e659d1
f6fc31d
22298e7
d0998b4
1930068
d52372f
0ddf50f
22c79f6
df2fd08
e120253
2929fab
4eedf4f
13916d8
aed0dc3
9dd885f
576c75c
52856ab
c78d7e1
8a18cb3
a9c6329
fe9e4c7
45114d8
2e1bdbd
651ac7e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it is public, this value can be read in place of
getTargetAssets
which would be interpreted incorrectly in caseenabled == 0
(because it seems it is expected to be interpreted as0
, pergetTargetAssets
's implementation)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think that it's better to enforce that the target assets is set back to zero before disabling the Plan. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this because it avoids side-effects which ultimately avoid unexpected/undesired behaviors
Is it compatible with the usual D3M life cycle though (awaiting confirmation from the Maker side ideally)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @hexonaut
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be able to disable without requiring target assets is 0 in the case the multisig goes rogue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but any account that's capable of disabling the plan is also capable of setting themselves as operator to set the target assets at zero right?
anyway the current version is also fine to me, but it has the little tradeoff that @Rubilmax is mentioning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or disabling the plan could also set back to zero target assets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in the way we have this constructed. This contract can trigger without a delay: https://github.com/makerdao/dss-direct-deposit/blob/master/src/D3MMom.sol#L76
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just make
targetAssets
aprivate
variable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to leave this for now unless you guys feel strongly. Code is in audit, and this is somewhat of a superficial change on a backend contract which can be upgraded later if desired.