Auto return stake in tg4 contract #124

hashedone · 2021-09-09T09:14:00Z

Closes #113
Closes #142 (via #146 merged in)

ethanfrey · 2021-09-09T09:55:23Z

contracts/tg4-stake/src/contract.rs


 use crate::error::ContractError;
 use crate::msg::{
    ExecuteMsg, InstantiateMsg, PreauthResponse, QueryMsg, StakedResponse, UnbondingPeriodResponse,
 };
 use crate::state::{members, Config, ADMIN, CLAIMS, CONFIG, HOOKS, PREAUTH, STAKE, TOTAL};

+pub type Response = cosmwasm_std::Response<TgradeMsg>;


ethanfrey

A few minor hints (that you probably know, but just writing it down)

ethanfrey · 2021-09-09T16:49:47Z

contracts/tg4-stake/src/contract.rs

+    let config = CONFIG.load(deps.storage)?;
+
+    if config.auto_return_limit > 0 {
+        let msgs = request_privileges(&[Privilege::EndBlocker]);


Nice to request this - and only if there is a reason to do so.

contracts/tg4-stake/src/contract.rs

ethanfrey · 2021-09-09T16:50:44Z

contracts/tg4-stake/src/contract.rs

+}
+
+fn end_block(deps: DepsMut) -> Result<Response, ContractError> {
+    // TODO: Auto return claims


And yes, this requires copying over Claims from cw-plus and modifying it with another index... tricky one to be sure.

FYI I'm doing copy of Claims in #139

You need to be merged first, so basically I would align to your changes with this.

maurolacy · 2021-09-10T16:24:41Z

contracts/tg4-stake/src/claim.rs

+}
+
+impl<'a> Claims<'a> {
+    pub fn new(storage_key: &'a str) -> Self {


contracts/tg4-stake/src/claim.rs

ethanfrey

Nice work. This looks mostly there.

The big issues I would raise are:

Using ADMIN directly from cw-plus v0.9.0 (which can be left as a TODO for another PR... your copy works fine for now, but let's remove code we don't need)
Better use of primary key. Indexes are awesome, you can make it even better 😄 ping me on discord if what I wrote didn't make sense
Merging in the work from Jakub from Store creation height for undelegate #139, so it doesn't get lost/overwritten

Happy to review tomorrow morning. This looks quite close to done

ethanfrey · 2021-09-14T15:14:53Z

contracts/tg4-stake/src/claim.rs

@@ -0,0 +1,162 @@
+//! This utility is based on `cw_controllers::Claim`, but tweaked to be more compliant to what is


This was already copied over (and modified) in #139 (which was now merged).

I would remove this commit (stash it?), rebase on top of origin/main, and then modify the version Jakub worked on. Otherwise, this is just asking for a merge conflict. (and loss of the functionality from #139)

Hmmm... this is quite some diff... maybe you cannot rebase, but need to merge it in and resolve conflicts manually

At morning I succesfully rebased it, but if there would be a problem I would just rebase manually, it should not be this much a pain (at least comments, other stuff may be).

ethanfrey · 2021-09-14T15:15:48Z

contracts/tg4-stake/src/claim.rs

+}
+
+struct ClaimIndexes<'a> {
+    pub addr: MultiIndex<'a, (Addr, Vec<u8>), Claim>,


This should be the Primary Key, we do not need a secondary index for this.
Just release_at as a secondary index

It was old code, not including my todays changes.

ethanfrey · 2021-09-14T15:17:35Z

contracts/tg4-stake/src/claim.rs

+
+pub struct Claims<'a> {
+    /// Claims are indexed by arbitrary numeric index, so every claim has its own entry
+    claims: IndexedMap<'a, U64Key, Claim, ClaimIndexes<'a>>,


Rather than an arbitrary U64Key, please use (&Addr, Timestamp) or such here.

Each secondary index has a rather non-trivial cost to read and write, let's make use of the primary key for an index when we can, rather than a meaningless counter here.

ethanfrey · 2021-09-14T15:19:10Z

contracts/tg4-stake/src/claim.rs

+                storage_key,
+                &format!("{}__addr", storage_key),
+            ),
+            release_at: MultiIndex::new(


Nice job constructing this.

ethanfrey · 2021-09-14T15:19:28Z

contracts/tg4-stake/src/claim.rs

+        };
+
+        let claims = IndexedMap::new(storage_key, indexes);
+        let next_key = Item::new(&format!("{}__key", storage_key));


Again, I would remove the next_key and just use the indexed map with a meaningful primary key

ethanfrey · 2021-09-14T15:21:06Z

contracts/tg4-stake/src/claim.rs

+                (Some(cap), Ok((_, sum))) => cap <= (*sum).into(),
+                _ => true,
+            })
+            // need to collet intermediately, cannot remove from map while iterating as it borrows


very nice chain here.

ethanfrey · 2021-09-14T15:21:54Z

contracts/tg4-stake/src/claim.rs

+            // take all claims for the addr
+            .range(storage, None, None, Order::Ascending)
+            // filter out non-expired claims (leaving errors to stop on first
+            .filter(|claim| match claim {


With compound index (Addr, U64Key), you can use the range query to only get expired Claims, and not every iterate over the others. more efficient

Code sample above

Yes, this is still inefficient. Using a .range(storage, None, end, Order::Ascending) where end is the current block time would be better.

True, I missed it here.

ethanfrey · 2021-09-14T15:23:30Z

contracts/tg4-stake/src/claim.rs

+            // removes item from storage and returns accumulated sum
+            .try_fold(0, |_, (idx, sum)| -> StdResult<_> {
+                self.claims.remove(storage, idx.into())?;
+                Ok(sum)


huh, don't you need to use sum + _ here?
actually, that scan confused me a bit above, maybe you don't.

No, scan already accumulated things, I would definetely simplify it.

ethanfrey · 2021-09-14T15:24:25Z

contracts/tg4-stake/src/contract.rs

@@ -73,7 +77,21 @@ pub fn execute(
    let api = deps.api;
    match msg {
        ExecuteMsg::UpdateAdmin { admin } => {
-            Ok(ADMIN.execute_update_admin(deps, info, maybe_addr(api, admin)?)?)
+            let resp = ADMIN.execute_update_admin(deps, info, maybe_addr(api, admin)?)?;


If you upgrade deps to cw-plus v0.9.0, you can use the ADMIN from cw-plus unmodified, right?

I missed this comment before, would apply it at evening/tomorrow, but it is not critical for logic

…s of passing regression)

ueco-jb · 2021-09-15T12:57:05Z

contracts/tg4-stake/src/claim.rs

-                Ok((_, claim)) => claim.release_at <= block.time,
+                Ok((_, claim)) => claim.release_at.is_expired(&block),


ueco-jb · 2021-09-15T13:07:18Z

contracts/tg4-group/src/state.rs

+use tg_controllers::Admin;
 use tg_controllers::{Hooks, Preauth};


Join those.

ueco-jb · 2021-09-15T13:07:50Z

contracts/tg4-mixer/src/contract.rs

    use cosmwasm_std::testing::{mock_env, MockApi, MockStorage};
-    use cosmwasm_std::{coins, Addr, Empty, Uint128};
+    use cosmwasm_std::{coins, Addr, Uint128};


Join those as well? I actually don't know what format are we using here.

cosmwasm_std and cosmwasm_std::testing are usually kept separately (test and non-test).

The other comment above I agree with

Hah, I suspected such.

ueco-jb · 2021-09-15T13:11:02Z

contracts/tg4-stake/src/claim.rs

-        creation_height: u64,
+        creation_heigh: u64,


It was correct (also in couple more places).

ueco-jb · 2021-09-15T13:17:15Z

contracts/tg4-stake/src/msg.rs

+const fn default_auto_return_limit() -> u64 {
+    20
+}


Why function instead of just variable? Won't compiler just inline it in the end anyway?

ueco-jb · 2021-09-15T13:22:20Z

packages/controllers/src/admin.rs

+        let control = Admin::new("foo");
+
+        // initialize and check
+        let admin = Some(Addr::unchecked("admin"));
+        control.set(deps.as_mut(), admin.clone()).unwrap();
+        let got = control.get(deps.as_ref()).unwrap();
+        assert_eq!(admin, got);


Nitpicking: I'd rename control to just admin and admin to hungarian (wink) admin_addr for better clarity
Then you could have
assert_eq!(admin_addr, admin.get(deps.as_ref()).unwrap());
(this got doesn't "speak" to me)

Also I have some issue here, without reading implementation it's not clear that those get/set methods are for reading/assigning addresses - maybe there's room for improvement?

If this is just admin copied from cw-plus, then let's make an issue there and let this continue as is.

Most likely, this file will be removed and we can just import the cw-controllers::Admin from cw-plus v0.9.0, which has support for generic Response

CosmWasm/cw-plus#424

ethanfrey

Looks quite good.

There are a number of comments for a follow up PR to improve the efficiency or code style.

I would just ask for 2 now, so we can merge it (and merge others on top).

heigh -> height properly named.
undo changes to tg4-group and tg4-mixer non-test code (keep them with Response)

when you finish those, I am happy to merge. And then you can start a new PR for the other issues.

ethanfrey · 2021-09-15T19:04:36Z

contracts/tg4-group/Cargo.toml

@@ -21,10 +21,11 @@ library = []
 [dependencies]
 cw0 = { version = "0.8.0" }
 cw2 = { version = "0.8.0" }
-cw-controllers = { version = "0.8.0" }
+cw-controllers = { version = "0.9.0" }


Hmm... this seems like a partial implementation of #142
I guess we merge main into #146 after this.

Update: I merged this in, worked fine. Now everything is up to v0.9.0

ethanfrey · 2021-09-15T19:05:41Z

contracts/tg4-group/src/contract.rs

@@ -14,6 +12,10 @@ use tg4::{
 use crate::error::ContractError;
 use crate::msg::{ExecuteMsg, InstantiateMsg, PreauthResponse, QueryMsg, SudoMsg};
 use crate::state::{members, ADMIN, HOOKS, PREAUTH, TOTAL};
+use tg_bindings::TgradeMsg;
+
+pub type Response = cosmwasm_std::Response<TgradeMsg>;


There is no need to use these types here.
I would use Response unless we make use of Tgrade features.
I think you only add them to tg4-stake.

ethanfrey · 2021-09-15T19:08:19Z

contracts/tg4-mixer/src/contract.rs

@@ -346,7 +349,7 @@ mod tests {
        }
    }

-    pub fn contract_mixer() -> Box<dyn Contract<Empty>> {
+    pub fn contract_mixer() -> Box<dyn Contract<TgradeMsg>> {


You don't need to change the contracts to make the types match.

There is ContractWrapper::new_with_empty, which takes a contract that returns Response and converts it to Contract for any T.

I'd prefer this over artificially adding the Tgrade Response types to tg4-group and tg4-mixer.

But yeah, in the multitest that uses them all (this file), we need to use adaptors so the App supports TgradeMsg (as I describe on the first line).

ethanfrey · 2021-09-15T19:08:46Z

contracts/tg4-mixer/src/contract.rs

@@ -382,7 +385,7 @@ mod tests {
    }

    // uploads code and returns address of group contract
-    fn instantiate_group(app: &mut App, members: Vec<Member>) -> Addr {
+    fn instantiate_group(app: &mut App<TgradeMsg>, members: Vec<Member>) -> Addr {


Yes, this is needed. (and all App changes)

ethanfrey · 2021-09-15T19:09:41Z

contracts/tg4-stake/src/claim.rs

    pub release_at: Expiration,
-    pub creation_height: u64,
+    /// Height of a blockchain in a moment of creation of this claim
+    pub creation_heigh: u64,


Please revert this change. We want the ending t

update: I fixed the spelling

ethanfrey · 2021-09-15T19:27:10Z

contracts/tg4-stake/src/contract.rs

-        QueryMsg::Claims { address } => {
-            to_binary(&CLAIMS.query_claims(deps, &deps.api.addr_validate(&address)?)?)
-        }
+        QueryMsg::Claims { address } => to_binary(&ClaimsResponse {


This will need to become a proper paginated list query... in a follow-up PR

ethanfrey · 2021-09-15T19:28:18Z

contracts/tgrade-valset/src/contract.rs

@@ -454,7 +454,7 @@ mod test {
    }

    fn contract_group() -> Box<dyn Contract<TgradeMsg>> {
-        let contract = ContractWrapper::new_with_empty(


This was a nice way to do it without forcing the upgrade to tg4-group.
I would undo any changes to that contract and just use new_with_empty here and in the mixer test.

ethanfrey · 2021-09-15T19:29:55Z

contracts/tg4-stake/src/claim.rs

+            .range(
+                storage,
+                None,
+                Some(Bound::inclusive(self.claims.idx.release_at.index_key((


I am pretty sure ExpirationKey::new(Expiration::now(block)).0 will work (or U64Key::from(block.time))

Can you make the change and see if tests pass.

ethanfrey · 2021-09-15T19:30:20Z

contracts/tg4-stake/src/claim.rs

+                Order::Ascending,
+            );
+
+        let mut claims = self.filter_claims(claims, None, limit.into())?;


we can remove this filter_claims, in this case, we always have the cash to cover them.

ethanfrey · 2021-09-15T19:31:04Z

contracts/tg4-stake/src/claim.rs

+        let mut claims = self.filter_claims(claims, None, limit.into())?;
+        claims.sort_by_key(|claim| claim.addr.clone());
+
+        let releases = claims


Nice grouping 👍

cw + 0.9.0 upgrade

ethanfrey

Okay, I made #148 for the type cleanup I want.

If you import TgradeMsg, you trigger another export that limits where the contract can deploy. I try to do that sparingly. But maybe you think it is better to make everything tied to Tgrade, and maybe you are right.

Take a look at it. You can merge #148 into this, and then this to main. Or just this into main as is.

I would like to get this in main. Follow up parts to finalise it are tracked in #147 and you can add to that.

ethanfrey · 2021-09-16T05:30:16Z

Reflecting some more... you are right using the TgradeMsg in all these contracts. This repo is designed to run on Tgrade and nowhere else. I am still in the generic cw-plus mindset.

I will close #148 and merge this.

contracts/tg4-stake/src/claim.rs

maurolacy · 2021-09-16T06:05:06Z

contracts/tg4-stake/src/claim.rs

+            // take all claims for the addr
+            .range(storage, None, None, Order::Ascending)
+            // filter out non-expired claims (leaving errors to stop on first
+            .filter(|claim| match claim {


Yes, this is still inefficient. Using a .range(storage, None, end, Order::Ascending) where end is the current block time would be better.

maurolacy · 2021-09-16T06:10:41Z

contracts/tg4-stake/src/claim.rs

+                Err(_) => true,
+            });
+
+        let claims = self.filter_claims(claims, cap.map(u128::from), None)?;


Why do we need a cap at all? A cap and a limit sound like too much / unnecessary to me.

cap came from my old code. I commented to remove this.

maurolacy · 2021-09-16T06:16:09Z

contracts/tg4-stake/src/claim.rs

+    }
+
+    /// Releases given claims by removing them from storage
+    fn release_claims(


I would call this remove_claims.

good idea. or maybe delete_claims

hashedone marked this pull request as draft September 9, 2021 09:14

ethanfrey reviewed Sep 9, 2021

View reviewed changes

maurolacy reviewed Sep 14, 2021

View reviewed changes

hashedone force-pushed the 113-auto-return-stake branch from 8b4ce43 to 74e9373 Compare September 14, 2021 10:57

ethanfrey reviewed Sep 14, 2021

View reviewed changes

hashedone force-pushed the 113-auto-return-stake branch from ffd1ea2 to d4fabbe Compare September 15, 2021 11:20

hashedone added 5 commits September 15, 2021 13:41

Registering for EndBlock in tg4-stage contract

5579d66

Upgrade of claim and admin in progress

938cd51

Cleaning up errors, making things compile

39d1089

Claims extended with claiming for arbitrary expired claims in tg4-stake

f56bfed

Corrected mistakes on rebase, make all current state working (in term…

2d0ac50

…s of passing regression)

hashedone force-pushed the 113-auto-return-stake branch from d4fabbe to 2d0ac50 Compare September 15, 2021 12:40

ueco-jb reviewed Sep 15, 2021

View reviewed changes

ueco-jb mentioned this pull request Sep 15, 2021

Refactor Admin cw-controller to better represent actual functionality CosmWasm/cw-plus#424

Closed

Final implementation of auto returning stake on endblock

743302e

hashedone force-pushed the 113-auto-return-stake branch from 8e9bec0 to 743302e Compare September 15, 2021 14:40

hashedone marked this pull request as ready for review September 15, 2021 14:41

hashedone and others added 6 commits September 15, 2021 17:02

Switched to using cw-controllers 0.9.0 Admins instead of custom one

8adc5ec

Schema updated

2816079

Update cw-+ deps to 0.9.0

17f5c4f

Updater lock file

1f2e414

Use new AppBuilder in multi tests

53db3ef

Fix lock file after rebase

e7d7621

ethanfrey reviewed Sep 15, 2021

View reviewed changes

ethanfrey added 2 commits September 15, 2021 21:43

Merge pull request #146 from confio/114-cw-+-0.9.0-upgrade

683aa39

cw + 0.9.0 upgrade

Fix typo heigh -> height

598676f

ethanfrey mentioned this pull request Sep 15, 2021

Improve tg4-stake auto-pay #147

Closed

7 tasks

ethanfrey approved these changes Sep 15, 2021

View reviewed changes

ethanfrey merged commit 60d467e into main Sep 16, 2021

ethanfrey deleted the 113-auto-return-stake branch September 16, 2021 05:30

maurolacy reviewed Sep 16, 2021

View reviewed changes

		@@ -0,0 +1,162 @@
		//! This utility is based on `cw_controllers::Claim`, but tweaked to be more compliant to what is

		Ok((_, claim)) => claim.release_at <= block.time,
		Ok((_, claim)) => claim.release_at.is_expired(&block),

		use tg_controllers::Admin;
		use tg_controllers::{Hooks, Preauth};

Auto return stake in tg4 contract #124

Auto return stake in tg4 contract #124

Conversation

hashedone commented Sep 9, 2021 • edited by ethanfrey Loading

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueco-jb Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey commented Sep 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashedone commented Sep 9, 2021 •

edited by ethanfrey

Loading

ueco-jb Sep 15, 2021 •

edited

Loading