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.

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

feat: migrate to ape and add features #16

Merged
merged 56 commits into from
Sep 4, 2023
Merged

Conversation

milkyklim
Copy link
Collaborator

@milkyklim milkyklim commented Aug 6, 2023

closes #15

Copy link
Member

@banteg banteg left a comment

Choose a reason for hiding this comment

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

looks very solid! thank you.

things to think about:

  • should we simplify funding to one transfer?
  • should we use blueprint vs minimal proxy?
  • should rug pullability be defined at creation? counterpoint: you can bundle multiple actions when deploying from a dao or safe.
  • should we change admin change to a single step?
  • it should be possible to recover the vested token sent to the contract minus still locked amount, add a test demonstrating that.

contracts/VestingEscrowFactory.vy Outdated Show resolved Hide resolved
contracts/VestingEscrowFactory.vy Outdated Show resolved Hide resolved
contracts/VestingEscrowSimple.vy Outdated Show resolved Hide resolved
contracts/VestingEscrowSimple.vy Outdated Show resolved Hide resolved
contracts/VestingEscrowSimple.vy Outdated Show resolved Hide resolved
contracts/VestingEscrowSimple.vy Outdated Show resolved Hide resolved
contracts/VestingEscrowSimple.vy Outdated Show resolved Hide resolved
contracts/VestingEscrowSimple.vy Outdated Show resolved Hide resolved
@banteg
Copy link
Member

banteg commented Aug 8, 2023

another decision:

  • open_claim added to init with True by default to enable smart contract recipients like 0xsplits.xyz
  • recipient can call set_open_claim(bool) which can set self.open_claim to False to disable this behavior
  • set_open_claim is a noop if the value doesn't change
  • emit SetOpenClaim(bool) if the value is changed
  • the check in claim() is changed to assert msg.sender == recipient or (self.open_claim and recipient == beneficiary)

@banteg
Copy link
Member

banteg commented Aug 8, 2023

writing the full spec in #17, it supersedes all the comments

log SetFree(admin)
log VestingTerminated(self.recipient, admin, ruggable, ts)
log Disowned(owner)
log Revoked(self.recipient, owner, ruggable, ts)
Copy link
Member

@banteg banteg Aug 9, 2023

Choose a reason for hiding this comment

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

we should also log beneficiary

also let's use time instead of ts

@milkyklim milkyklim force-pushed the level-up branch 7 times, most recently from 805b41a to 4a21429 Compare August 14, 2023 22:26
@milkyklim milkyklim force-pushed the level-up branch 7 times, most recently from 58d5659 to afaadcb Compare August 26, 2023 13:23
@milkyklim milkyklim force-pushed the level-up branch 2 times, most recently from cf7ffbf to 97a55db Compare August 31, 2023 14:02
@milkyklim milkyklim changed the title feat: migrate to ape and vyper blueprint feat: migrate to ape and add features Sep 1, 2023
@milkyklim milkyklim force-pushed the level-up branch 2 times, most recently from 291a791 to 2763112 Compare September 4, 2023 10:52
@banteg banteg merged commit 945b5ca into yearn:master Sep 4, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rewrite to ape and update
2 participants