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

Vt vesting admin #7

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

vernonjohnson
Copy link

No description provided.

@the-frey
Copy link

the-frey commented Jan 5, 2022

Looks good. Config struct makes sense for this.

Only question:

So on the spike I spent a few minutes on yday there's a mapping to halt payments, cos I figured that you might want to halt payments to only a single recipient.

However, having a global halt on payments makes sense if there's only ever likely to be one recipient for all payments specified in the sequence of payments passed to instantiate 🤔

Just asking to ask, not sure I have a strong opinion unless the future intention is multiple recipients.

@vernonjohnson
Copy link
Author

Yeah I think these could be used for multiple recipients. Sounds like that would be pretty common use case. So the approach you mentioned may work better for that.

@ben2x4
Copy link
Owner

ben2x4 commented Jan 6, 2022

Hey y'all! Awesome you are jumping into work on this contract. I'm curious about the choice to use halt payments, instead of refund? Are you thinking we would eventually have both?

Also great questions around single vs multiple recipients. Single recipient will lead to the simplest code and with cosmwasm is easy enough to spin up many contracts. Though initialization is probable easier if we allow multiple recipients. Curious on your thoughts.

@vernonjohnson
Copy link
Author

@ben2x4 Added some revisions:

  • Adding new payments
  • Stopping individual payments
  • Instantiating with owner + updating owner
    Lmk what you think

@the-frey
Copy link

LGTM from a scan through. Will give it another check later.

@ben2x4 could you approve and run the workflow pls 🙏 don't have perms...

@vernonjohnson
Copy link
Author

@the-frey Appreciate the review. @ben2x4 lmk what you think.

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.

3 participants