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

Cw4 stake to control valset #29

Merged
merged 18 commits into from
Apr 15, 2021
Merged

Cw4 stake to control valset #29

merged 18 commits into from
Apr 15, 2021

Conversation

maurolacy
Copy link
Contributor

Closes #6.

Based on #5 (cw4-group) tests. Tests are a little redundant and verbose in the name of clarity.

The pair cw4-stake + valset seems to be working well.

Will now add some unit tests to solve the TODOs from #5. Not sure what else needs to be tested.

@maurolacy maurolacy added the multi-test To flag issues / code / MRs that require / implement multi-tests label Apr 14, 2021
@maurolacy maurolacy self-assigned this Apr 14, 2021
@maurolacy maurolacy requested a review from ethanfrey April 14, 2021 07:35
Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Stake test looks good.
I would definitely prefer literal values for the expected results, but the actual setup and expectations is correct.

contracts/tgrade-valset/src/test_valset_stake.rs Outdated Show resolved Hide resolved
contracts/tgrade-valset/src/test_valset_stake.rs Outdated Show resolved Hide resolved
contracts/tgrade-valset/src/test_valset_stake.rs Outdated Show resolved Hide resolved
contracts/tgrade-valset/src/test_valset_stake.rs Outdated Show resolved Hide resolved
@ethanfrey
Copy link
Contributor

Will now add some unit tests to solve the TODOs from #5. Not sure what else needs to be tested.

I assume this is still pending? And same comment with using more literals for the expected results (and smaller groups)

@maurolacy
Copy link
Contributor Author

Will now add some unit tests to solve the TODOs from #5. Not sure what else needs to be tested.

I assume this is still pending?

It is. Working on it.

@maurolacy maurolacy force-pushed the cw4-stake-controls-valset branch from ae31e28 to e8e670a Compare April 14, 2021 10:52
Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Nice improvement.
The same can be done in some of the other tests in contract.rs when you work on those.

@maurolacy maurolacy force-pushed the cw4-stake-controls-valset branch from 0dfac7a to 8f950a7 Compare April 14, 2021 16:16
@maurolacy maurolacy force-pushed the cw4-stake-controls-valset branch from 8f950a7 to 7c90ec7 Compare April 14, 2021 20:00
@maurolacy
Copy link
Contributor Author

maurolacy commented Apr 14, 2021

I was trying to do the end_block_run tests, and my idea was to use sudo to send the TgradeSudoMsg::EndWithValidatorUpdate message to call end_block, but for some reason, that is failing with:

`Result::unwrap()` on an `Err` value: "Error parsing into type alloc::string::String: Invalid type"

So, it seems to me that multi-test sudo is broken. Or, I'm using it incorrectly.

In any case, I think this can be reviewed and merged as-is. And I'll open a new issue to debug / fix multi-test sudo.

@maurolacy
Copy link
Contributor Author

maurolacy commented Apr 14, 2021

Oh, I solved it. It's just that I need a ContractWrapper created with new_with_sudo.

It would be nice to improve on the error message, though. It took me a couple of hours (including patching crates and so on) to find and realize this.

Update: See CosmWasm/cw-plus#278.

Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good. Let's merge it.

We can add more tests later when we optimize the cw4 calls, but this is good sanity checks

@@ -577,9 +538,151 @@ mod test {
assert_eq!(expected, active.validators);
}

// TODO: validator list (and modifications)
#[test]
fn validator_list() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for covering this

)
.unwrap();

// End block has run now, so active validators list is updated
Copy link
Contributor

Choose a reason for hiding this comment

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

perfect

@ethanfrey ethanfrey merged commit de020ef into main Apr 15, 2021
@ethanfrey ethanfrey deleted the cw4-stake-controls-valset branch April 15, 2021 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multi-test To flag issues / code / MRs that require / implement multi-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Control validator set with cw4-stake contract
2 participants