Add new Staking messages and queries under feature flag #300
Conversation
ethanfrey
force-pushed
the
new-staking-messages
branch
from
6fef990 to
a15b22b
Compare
webmaster128
left a comment
webmaster128
left a comment
There was a problem hiding this comment.
Looks good as far as I can see. I just have mixed feelings about the introduction of all those feature flags. The iterator alone is annoyance, and we're working towards many more.
.circleci/config.yml
Outdated
| command: cargo wasm --locked --features iterator | ||
| - run: | ||
| name: Run unit tests (with iterator support) | ||
| name: Run unit tests (with iterator and staking support) |
There was a problem hiding this comment.
So you want to test minimal and full feature set (and skip default)? Would be good to describe that in the steps somehow. Also there is --all-features available. I don't have a brilliant idea how to origanize all of this, but is getting complex now.
There was a problem hiding this comment.
Yeah, these features are independent, so I was thinking all or nothing. Sometimes the code doesn't work with the cfg feature blocks missing. (I had that happen a few times).
There was a problem hiding this comment.
I like the --all-features flag and used it here: b7b1ac7
|
I agree with limiting feature flags. I would be careful to add more, but I also think if we get some dynamic check from #301 that maybe we can remove the compiler feature for staking and add that as a runtime argument controlling upload of contracts. I agree this is suboptimal, and we definitely don't want to add 10 more. |
ethanfrey
force-pushed
the
new-staking-messages
branch
from
b4ab089 to
4c9aaef
Compare
|
@webmaster128 This should be complete now. Let me know what you think. The actual types we used have been discussed with @yys in #211, so the question is mainly on the organization (and feature flags). As I said above, I would love to remove the feature flag when #301 is in and we have another way to check for optional features. |
packages/std/src/init_handle.rs
Outdated
| // delegator is automatically set to address of the calling contract | ||
| validator: HumanAddr, | ||
| // this is the "withdraw address", the one that should receive the rewards | ||
| // if None, then use delegator address |
There was a problem hiding this comment.
Can we make such field-specific comments doc comments (///) to allows IDEs and JSON schema to utilize them? Note that when rendered there is no resulting line break as long as it is the same paragraph, so a trailing . would be good.
Of course, this is not for // delegator is automatically set to address of the calling contract, which is not a field description.
packages/std/src/mock.rs
Outdated
| pub fn new(balances: &[(&HumanAddr, &[Coin])]) -> Self { | ||
| MockQuerier { | ||
| bank: BankQuerier::new(balances), | ||
| staking: staking::StakingQuerier::new(&[], &[]), |
There was a problem hiding this comment.
This could probably be staking::StakingQuerier::default()
packages/std/src/query.rs
Outdated
| #[cfg(feature = "staking")] | ||
| #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)] | ||
| #[serde(rename_all = "snake_case")] | ||
| /// DelegationsResponse is data format returned from StakingRequest::Delegations query |
There was a problem hiding this comment.
Side note: Rust will most likely converge towards doc comments first. The reasoning is to better support long doc comments, that otherwise move other attributes out of the screen. This is specified but not yet implemented in rustfmt: rust-lang/rustfmt#3303
No issue here as tooling will sort them for us one day and we don't typically have long doc comments.
There was a problem hiding this comment.
Nice note, I have no opinion, put them close to the struct to make sure they bind, but the other way (doc comments first) does look cleaner.
|
Thanks for the tips, I will keep the rustdoc more in mind. I really need to set up (and review) the public rustdoc for this package. Would make a nice last-minute pre-0.8 polish. |
Any idea if it is possible to publish pre-releases to crates.io? I did not find a possibility without overriding the "latest" version. |
AFAIK the latest version is always taken, so not really a "pre-release". I guess we could call it We can also check rustdoc locally and make sure the 0.7.2 is published, to prepare for the update, if that is why you are asking. |
|
Publishing a pre-release like |
Replaces #211
Closes #174
I think everything is now implemented except for the test cases. Happy for a review and feedback,
MockQuerier, especiallyStakingQuerierFor another PR:
Note: I like the feature flag approach, as it allows us to downgrade and support non-PoS chains easily (eg. if someone wants to run cosmwasm in a PoA environment or other novel consensus mechanism.). However, I realize that the schemas generated for
cosmwasm_stdare now incomplete.... We can either generate them with or without staking info.My idea (which I tentatively did in a15b22b) is to make staking on by default and use
--no-default-featuresto disable it, as it will be the case in almost all cosmwasm chains in the near future. And the other chains can just ignore some entries in the schema (rather than having missing specs). Open for more ideas.