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

Dao uses Stakeable token #93

Merged
merged 18 commits into from
Dec 7, 2021
Merged

Dao uses Stakeable token #93

merged 18 commits into from
Dec 7, 2021

Conversation

ben2x4
Copy link
Contributor

@ben2x4 ben2x4 commented Dec 2, 2021

This PR implements the stakeable token into the DAO. The basic architectures is as follows

CW20-Stakeable
This is a extension of cw20-base that lets the token be staked and staked balances to be queried at any arbitrary height. The contract can set an unbonding period which behaves like native cosmos sdk unbonding. The users pick an amount to unbond and then must wait the unbonding period to be able to claim the asset. While staked, assets are non-transferable.

CW20-Gov
This is an extension of CW20-Stakeable that add voting power delegation features. Staked tokens count towards voting power and stakers can delegate their voting power to an address of their choosing. Like the previous implementation, delegation is all or nothing, ie the user can designate one address to vote with all their voting power with no ability to make partial delegations.

CW-DAO
This contract uses the new CW20-Gov. This means memebers of the DAO must stake their tokens in order to be able to vote. This required updating some of the balance queries, tests, and gov token instantiation messages.

Advantages

The main advantage of this approach is enabling efficient distribution of assets to DAO stakers and quorem calculations. All tokens trading in the market will have some portion of supply locked in various defi protocols such as swaps or lending platforms. These contracts most likely will not be able to receive distributions and thus a portion of all assets distributed would in effect be burned. Under the staking method. users and contracts most actively stake their tokens in the DAO in order to be eligible for these distributions. Quorem will also be much easier to calculate as it is much safer to make the assumption that stakers are physically capable of voting on proposals. DAOs that want to make it easy to transfer assets can choose to have no unbonding period so unbonded assets instantly available for transfer.

Open Questions

Deposits are a little tricky with the new implementation. Deposits must be made with unstaked assets. I think this is a fine solution and I believe a fix for this may make the cw20-gov and cw-dao contracts too interdependent. Though curious to hear thoughts.

@ben2x4 ben2x4 linked an issue Dec 2, 2021 that may be closed by this pull request
@JakeHartnell
Copy link
Member

How do I provide tokens to be claimed? I see the ability to query claims, but I don't see where they're set...

Also, this needs a quick rebase. : )

@JakeHartnell
Copy link
Member

How do I provide tokens to be claimed? I see the ability to query claims, but I don't see where they're set...

Also, this needs a quick rebase. : )

My bad, I misunderstood this. Claims not rewards, its just for unbonded tokens... was thinking of how to provide rewards for staking gov tokens.

Still needs rebase. : )

Copy link
Contributor

@0xekez 0xekez left a comment

Choose a reason for hiding this comment

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

This is cool :) Nice to see how much code was removed.

@@ -40,74 +51,57 @@ pub fn execute(
) -> Result<Response<Empty>, ContractError> {
match msg {
ExecuteMsg::Transfer { recipient, amount } => {
cw20_base::contract::execute_transfer(deps, _env, info, recipient, amount)
.map_err(ContractError::Cw20Error)
execute_transfer(deps, _env, info, recipient, amount).map_err(ContractError::Cw20Error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we use it _env should probably be renamed to env.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

});
Ok(balance.balance)
);
let balance = res.map(|r| r.balance).unwrap_or_else(|_| Uint128::zero());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it the case that for all types of StdError we still want to return a balance of zero? Why not propagate the error up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a great catch. The contract we are querying already handles the case that there is not data for the address and returns 0 balance as appropriate. Changed the code to propagate the error up.

@@ -53,6 +56,13 @@ pub enum ExecuteMsg {
marketing: Option<String>,
},
UploadLogo(Logo),
Stake {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add some doc comments to these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@JakeHartnell
Copy link
Member

Please bump package versions. 🙏

Otherwise, LGTM.

Copy link
Member

@JakeHartnell JakeHartnell left a comment

Choose a reason for hiding this comment

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

🚢

@0xekez 0xekez merged commit 8a8dd33 into main Dec 7, 2021
@the-frey the-frey deleted the dao-uses-stakeable-token branch December 7, 2021 09:28
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.

Staking
3 participants