Lockup module

[antstalepresh]

No description provided.

[Thunnini]

Generally, I think this is good. However, one thing we should consider is to prevent users accidentally locking their tokens for a wrong length. For example, the ‘yield’ module may only support 3, 6, 9 month lockup periods. But if the user happens to accidentally send a transaction for a 4-month locking period they will have to wait 4 months to be able to do anything with their token while not receiving rewards. As this is the user’s own fault, it may not be a crucial consideration for launch but is worth considering.

Imho, I think the lockup module could avoid using hooks and not have any messages, and rather use a way where it works from the other module’s keepers. So the other module’s keeper can lock and unlock token from the lockup keeper?

[sunnya97]

@Thunnini

However, one thing we should consider is to prevent users accidentally locking their tokens for a wrong length. For example, the ‘yield’ module may only support 3, 6, 9 month lockup periods...

I think this would be better to handle at the UI level, don't need to unnecessarily restrict lockup times in the state machine, because there might be use cases we might not have thought of yet. For example, another project might want to have liquidity mining for their own pools, based on times that aren't used by the base yield module. Maybe I just want to reward people for providing 1 week liquidity locks for my token :)

Also, another use case for this module I was thinking of is validators to lockup their staking derivatives to give guarantees to their delegators. Because if staking automatically creates staking derivatives for example, validators may want to lockup their derivatives to show delegators that they are long term locked and invested.

Imho, I think the lockup module could avoid using hooks and not have any messages, and rather use a way where it works from the other module’s keepers. So the other module’s keeper can lock and unlock token from the lockup keeper?

This seems like an interesting design pattern. But I'm worried this comes back to restricting "un-thought of" use cases. Someone might come up with a use case for the lockup module that's unrelated to any existing module on the chain. (For example, the lockdrops done by Edgeware).

Oh also, for our use case, can't locking up tokens have multiple uses? For example, I could be locking some LP tokens to vote in my LP pool governance, but also for yield farming. Which module's lockMsg should I be using? I'd still need hooks regardless.

[Thunnini]

Most of the the code looks good, but I had a couple questions on the following points. Could you take a look @antstalepresh?

[sunnya97]

Should these be public functions?

[antstalepresh]

It's used within test

[sunnya97]

Oh I didn't realize tests in the same package, couldn’t access private functions. Is there another way to deal with this, because otherwise it breaks the point of separating AdminKeeper from normal.