Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add vat.live check to DssVestSuckable pay #48

Merged
merged 26 commits into from
Apr 19, 2022
Merged

Add vat.live check to DssVestSuckable pay #48

merged 26 commits into from
Apr 19, 2022

Conversation

naszam
Copy link
Contributor

@naszam naszam commented Mar 21, 2022 •
edited
Loading

  • Add vat.live check to DssVestSuckable pay
  • Refactor Tests
    • fetch from ChainLog
    • split interfaces for contract and tests
  • Add Fuzz Coverage (Update Echidna Tests)
  • Add FV Coverage (Update Certora Specs)
  • Document vat.suck circuit breaker in the readme
  • Minor Changes

@gbalabasquer
Copy link
Contributor

gbalabasquer commented Mar 21, 2022 •
edited
Loading

Hmmm well this is a circuit breaker that requires keeper intervention. We should evaluate if in this case the extra storage load is too sensitive.
If we follow this path I'd recommend the cage naming.

@brianmcmichael
Copy link
Contributor

Hmmm well this is a circuit breaker that requires keeper intervention. We should evaluate if in this case the extra storage load is too sensitive.
If we follow this path I'd recommend the cage naming.

Agree cage() might be a more appropriate name for MCD than kill(),
however I really liked this solution as it uses the existing mutex variable and adds no additional costs to any of the existing calls and this will effectively lock the entire contract permissionlessly with the flip of a switch.

@naszam naszam changed the title kill cage Mar 22, 2022
@brianmcmichael
Copy link
Contributor

Still some backchannel discussion over kill() 👹 🔪 😅 vs. cage() ⛓️ 😅 ⛓️

And this solution potentially adds some more keeper overhead despite the permissionless nature of the function, we should probably have a bigger cage match where we hammer out a policy around pushing new keeper infrastructure.

@talbaneth
Copy link
Contributor

Hmmm well this is a circuit breaker that requires keeper intervention. We should evaluate if in this case the extra storage load is too sensitive. If we follow this path I'd recommend the cage naming.

You guys might have finalized on this and if so it's ok, but note that the dai vest contract had only 16 calls in the last 30 days so maybe the cost of doing an external read on each vest is neglible and we can avoid the need for a keeper.
(BTW like the use of the lock with the current solution, indeed elegant).

@naszam
Copy link
Contributor Author

naszam commented Mar 24, 2022

I've included implementation approaches, following up with Gonza in chat, related to what discussed so far, including a keeper-free, authed cage relying on lock check (that would require cage update in dss end) and vat.live check on suckable pay function that adds a bit of extra gas costs on vest calls but avoid the need of a keeper.
https://hackmd.io/@naszam/Sy1SOo1M9

@gbalabasquer
Copy link
Contributor

I think specifically for dss-vest I prefer to do the vat.live() == 1 check.

@naszam
Copy link
Contributor Author

naszam commented Mar 25, 2022

based on the comments so far, I think emerging consensus is leaning towards a vat.live() == 1 check in DssVestSuckable pay as it won't require the need for extra keeper support or a cage update in dss end at the little expense of a bit extra gas cost on vest calls.

@brianmcmichael
Copy link
Contributor

I think specifically for dss-vest I prefer to do the vat.live() == 1 check.

I'm ok with this. Usage of this is light and this gives peace of mind.

@naszam naszam changed the title cage Add vat.live check to DssVestSuckable pay Mar 28, 2022
@naszam naszam marked this pull request as ready for review March 30, 2022 17:14
@naszam
Copy link
Contributor Author

naszam commented Mar 31, 2022

@brianmcmichael @gbalabasquer @talbaneth The PR should be ready for review.
There is still some fuzz testing related work to do, that will be addressed in a separate PR eventually.

talbaneth
talbaneth reviewed Apr 4, 2022
View reviewed changes
src/DssVest.t.sol Outdated Show resolved Hide resolved
talbaneth
talbaneth reviewed Apr 4, 2022
View reviewed changes
Copy link
Contributor

@talbaneth talbaneth left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look ok to me, see small comments.

Not sure if this was debated but I'll mention anyway - this change does have a social aspect as it means ES truncates also vested (but unclaimed) dai immediately.
We can theoritaclly use End.when() and End.wait() to still allow payments up to when End.thaw is possible (with the cost of introducing dependency on End).

gbalabasquer
gbalabasquer reviewed Apr 11, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
gbalabasquer
gbalabasquer reviewed Apr 11, 2022
View reviewed changes
src/DssVest.t.sol Outdated Show resolved Hide resolved
talbaneth
talbaneth approved these changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@talbaneth talbaneth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
In general I think it's preferable next time to separate the Echina version upgrade and the disabling of the time mutable tests to separate PRs.

gbalabasquer
gbalabasquer approved these changes Apr 15, 2022
View reviewed changes
Copy link
Contributor

@gbalabasquer gbalabasquer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

brianmcmichael
brianmcmichael approved these changes Apr 19, 2022
View reviewed changes
Copy link
Contributor

@brianmcmichael brianmcmichael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@naszam naszam merged commit 9a016b2 into dev Apr 19, 2022
gbalabasquer added a commit that referenced this pull request Apr 28, 2022
@gbalabasquer @naszam
Add vat.live check to DssVestSuckable pay + Follow DSS Slot Patte…
4e04cec 
…rn + Switch modifier order + others

* Switch modifier order (#42)

* Follow DSS Slot Pattern (#43)

* Add `vat.live` check to DssVestSuckable `pay` (#48)

* `kill`

* tests(refactor): fetch from chainlog

* rename to `cage`

* tests(refactor): split interfaces for contract and tests + cleanups

* tests(refactor): keep ds-token and gem/dai interfaces separated

* suckable(breaker): add `vat.live` check in `pay`

* echidna(suckable): add `mutlive` + `vat.live` revert case in `vest` and `vest_amt`

* echidna(`v2.0.0`): update config and readme

* certora(suckable): add `vat.live` revert case in `vest_revert` and `vest_amt_revert`

* readme(suckable): document `vat.suck` circuit breaker

* echidna(fix): extend `mutlock` behaviour

* echidna(fix): address `add` warnings + remove math `sub` where unused

* vest(errmsg): rename error messages to vest-specific contract

* tests(refactor): replace `Award` with `DssVest.Award`

* echidna(refactor): replace `Award` with `DssVest.Award`

* echidna(fix): address math warnings + remove unused math

* transferrable(errmsg): add missing error message in `pay` check

* echidna(transferrable): update `vest` and `vest_amt` with transfer error message

* readme(review): disable `vest()`

* echidna(transferrable): fix `vest` and `vest_amt` catch error block

* echidna(transferrable): remove transfer error message as won't be reached

* echidna(transferrable): cleanup

* echidna(mutations): disbale time-based fuzz mutations

* tests(doc): add comment for `vat.sin(VOW)` check

Co-authored-by: Nazzareno Massari <nazzareno@nazzarenomassari.com>
Co-authored-by: Nazzareno Massari <nas@naszam.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@talbaneth talbaneth talbaneth approved these changes

@gbalabasquer gbalabasquer gbalabasquer approved these changes

@brianmcmichael brianmcmichael brianmcmichael approved these changes

@kmbarry1 kmbarry1 Awaiting requested review from kmbarry1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@naszam @gbalabasquer @brianmcmichael @talbaneth