-
Notifications
You must be signed in to change notification settings - Fork 21
Conversation
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.
Two minor changes, but otherwise looks good.
|
||
/** | ||
@dev Sets the target rate threshold for a dai direct deposit module (d3m) | ||
@dev Aave: Targets the variable borrow rate |
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.
Same here. Let's leave this generic to both Aave/Compound and whoever else in the future.
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 think we should add a line for Compound later. This is to specifically point out that for Aave, we're targeting the borrow rate (and not the lending rate). Every protocol is going to be different.
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 suspect it will be the same. Really we could pick either in every lending market, but we are interested in the borrow rate as that is what is competitive with Maker.
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'm okay if you want to add a comment for every D3M type though.
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.
Checked:
lib/dss-direct-deposit
points to773069
(not latest on master)- README Changes Look Good
- DssExecLib D3M Action Looks Good
- Aave Mock Looks Good
- Tests Look Good
- Tests PASS
nit: noticed a recent commit on the d3m repo, we could consider to dapp upgrade dss-direct-deposit
to latest on master and eventually consider to release a new d3m version @hexonaut (as some minor changes in code have been introduced since the first release) and point to the release tag.
LGTM 👍
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.
LGTM. Noted about the dss-direct-deposit version @naszam
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.
LGTM
Latest master on D3M is just a little change I made to README. No functional difference. |
Adds function for setting D3M
bar
to the DssExecLib