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.

Sign up for GitHub

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

feat: Add proto annotation for deterministic queries #13174

Merged
merged 7 commits into from
Sep 8, 2022
Merged

feat: Add proto annotation for deterministic queries #13174

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0968bc5
feat: Add proto annotation for deterministic queries
amaury1093 Sep 7, 2022
bb88195
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/130…
amaury1093 Sep 7, 2022
715f2cb
Add comment
amaury1093 Sep 7, 2022
26d2bc8
Tweak comment
amaury1093 Sep 7, 2022
6632a3a
protocgen
amaury1093 Sep 7, 2022
2e95c03
Add gas
amaury1093 Sep 8, 2022
831b94b
Merge branch 'main' into am/13041-deterministic-proto
amaury1093 Sep 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions proto/cosmos/auth/v1beta1/query.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import "google/protobuf/any.proto";
import "google/api/annotations.proto";
import "cosmos/auth/v1beta1/auth.proto";
import "cosmos_proto/cosmos.proto";
import "cosmos/query/v1/query.proto";

option go_package = "github.com/cosmos/cosmos-sdk/x/auth/types";

Expand All @@ -16,49 +17,57 @@ service Query {
//
// Since: cosmos-sdk 0.43
rpc Accounts(QueryAccountsRequest) returns (QueryAccountsResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/auth/v1beta1/accounts";
}

// Account returns account details based on address.
rpc Account(QueryAccountRequest) returns (QueryAccountResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/auth/v1beta1/accounts/{address}";
}

// AccountAddressByID returns account address based on account id
rpc AccountAddressByID(QueryAccountAddressByIDRequest) returns (QueryAccountAddressByIDResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/auth/v1beta1/address_by_id/{id}";
}

// Params queries all parameters.
rpc Params(QueryParamsRequest) returns (QueryParamsResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/auth/v1beta1/params";
}

// ModuleAccounts returns all the existing module accounts.
//
// Since: cosmos-sdk 0.46
rpc ModuleAccounts(QueryModuleAccountsRequest) returns (QueryModuleAccountsResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/auth/v1beta1/module_accounts";
}

// Bech32Prefix queries bech32Prefix
//
// Since: cosmos-sdk 0.46
rpc Bech32Prefix(Bech32PrefixRequest) returns (Bech32PrefixResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/auth/v1beta1/bech32";
}

// AddressBytesToString converts Account Address bytes to string
//
// Since: cosmos-sdk 0.46
rpc AddressBytesToString(AddressBytesToStringRequest) returns (AddressBytesToStringResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/auth/v1beta1/bech32/{address_bytes}";
}

// AddressStringToBytes converts Address string to bytes
//
// Since: cosmos-sdk 0.46
rpc AddressStringToBytes(AddressStringToBytesRequest) returns (AddressStringToBytesResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/auth/v1beta1/bech32/{address_string}";
}
}
Expand Down
11 changes: 11 additions & 0 deletions proto/cosmos/bank/v1beta1/query.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,21 @@ import "google/api/annotations.proto";
import "cosmos/base/v1beta1/coin.proto";
import "cosmos/bank/v1beta1/bank.proto";
import "cosmos_proto/cosmos.proto";
import "cosmos/query/v1/query.proto";

option go_package = "github.com/cosmos/cosmos-sdk/x/bank/types";

// Query defines the gRPC querier service.
service Query {
// Balance queries the balance of a single coin for a single account.
rpc Balance(QueryBalanceRequest) returns (QueryBalanceResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/bank/v1beta1/balances/{address}/by_denom";
}

// AllBalances queries the balance of all coins for a single account.
rpc AllBalances(QueryAllBalancesRequest) returns (QueryAllBalancesResponse) {
option (cosmos.query.v1.deterministic) = true;
Copy link
Member

Choose a reason for hiding this comment

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

I would argue this is the type of query that we should be very cautious about exposing to the state machine. Without pagination it very gas intensive.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is true. But high gas costs, which smart contract devs should be aware of, is technically orthogonal to determinism. How do we express this to developers?

Copy link
Member

@aaronc aaronc Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure.. the main point to communicate with the annotation should be "appropriate for queries from modules" which is both determinism + manageable gas.

I'm assuming this particular query does have proper gas tracking via the store so it would likely just run out of gas. But I do also question the usefulness of these expensive queries. I guess this query is less expensive than querying all holders of a denom (which is what I thought it was), but even there, there is a scenario where a module might want to airdrop to all holders of a coin so I guess that is a valid use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose then cosmos.query.v1.deterministic is a misnomer

Copy link
Member

Choose a reason for hiding this comment

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

@AmauryM any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

ACK. Just leave it as-is and we can add warning comments saying gas could be high.

Copy link
Member

@aaronc aaronc Sep 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thinking about this a bit more, I actually think we should rename.

The three of us have the understanding regarding gas consumption, but we can't assume that about future users who might not read the docstring carefully. I think it is safer to use something like module_query_safe which prompts users to think more carefully about what "safe" means and if they don't know they're likely to check the doc string. Most people think they know what determinism means and then mark a query as such without thinking about other potential issues.

I'm actually thinking of some concrete examples right now where I want to add queries around protobuf file descriptors which are probably deterministic but wouldn't track gas and I would want to mark them explicitly as module_query_safe = false.

Copy link
Contributor Author

@amaury1093 amaury1093 Sep 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

which prompts users to think more carefully about what "safe" means and if they don't know they're likely to check the doc string

Or maybe the opposite might happen: module developer sees module_query_safe = true on a high-gas query, and assumes it's safe to use, and ends up with unexpected high gas consumption.

If I want the user to check doc strings, I would actully put unsafe_, or expert_. unsafe_ wouldn't work here though, it has a different meaning.

I'm not opposed to Bez's idea of having 2 annotations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just want to note again, that "high gas consumption" is relative to the chain. What one chain might consider high, another chain might deem as normal.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Clearly the bigger issue is computation without gas consumption

option (google.api.http).get = "/cosmos/bank/v1beta1/balances/{address}";
}

Expand All @@ -27,32 +30,38 @@ service Query {
//
// Since: cosmos-sdk 0.46
rpc SpendableBalances(QuerySpendableBalancesRequest) returns (QuerySpendableBalancesResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/bank/v1beta1/spendable_balances/{address}";
}

// TotalSupply queries the total supply of all coins.
rpc TotalSupply(QueryTotalSupplyRequest) returns (QueryTotalSupplyResponse) {
option (cosmos.query.v1.deterministic) = true;
Copy link
Member

Choose a reason for hiding this comment

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

ditto

option (google.api.http).get = "/cosmos/bank/v1beta1/supply";
}

// SupplyOf queries the supply of a single coin.
rpc SupplyOf(QuerySupplyOfRequest) returns (QuerySupplyOfResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/bank/v1beta1/supply/by_denom";
}

// Params queries the parameters of x/bank module.
rpc Params(QueryParamsRequest) returns (QueryParamsResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/bank/v1beta1/params";
}

// DenomsMetadata queries the client metadata of a given coin denomination.
rpc DenomMetadata(QueryDenomMetadataRequest) returns (QueryDenomMetadataResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/bank/v1beta1/denoms_metadata/{denom}";
}

// DenomsMetadata queries the client metadata for all registered coin
// denominations.
rpc DenomsMetadata(QueryDenomsMetadataRequest) returns (QueryDenomsMetadataResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/bank/v1beta1/denoms_metadata";
}

Expand All @@ -61,6 +70,7 @@ service Query {
//
// Since: cosmos-sdk 0.46
rpc DenomOwners(QueryDenomOwnersRequest) returns (QueryDenomOwnersResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/bank/v1beta1/denom_owners/{denom}";
}

Expand All @@ -72,6 +82,7 @@ service Query {
//
// Since: cosmos-sdk 0.47
rpc SendEnabled(QuerySendEnabledRequest) returns (QuerySendEnabledResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/bank/v1beta1/send_enabled";
}
}
Expand Down
19 changes: 19 additions & 0 deletions proto/cosmos/query/v1/query.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
syntax = "proto3";

package cosmos.query.v1;

import "google/protobuf/descriptor.proto";

// TODO: once we fully migrate to protov2 the go_package needs to be updated.
// We need this right now because gogoproto codegen needs to import the extension.
option go_package = "github.com/cosmos/cosmos-sdk/types/query";

extend google.protobuf.MethodOptions {
// deterministic is set to true when the query is guaranteed to return a
// deterministic response. Concretely, it means that the query has been
// tested on the SDK to return the exact same response upon multiple calls.
//
// When set to true, the query can safely be called from within the state
// machine, for example via ADR-033 calls or from CosmWasm contracts.
bool deterministic = 11110001;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fyi in #12972, @aaronc you proposed cosmos.query.v1.internal.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah not quite sure what the right word is

Copy link
Member

Choose a reason for hiding this comment

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

We do need to communicate with the doc string (and possibly the name) that these queries need to do proper gas management. Determinism is necessary but not sufficient for ADR-033

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a great point!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

}
15 changes: 15 additions & 0 deletions proto/cosmos/staking/v1beta1/query.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,89 +6,104 @@ import "gogoproto/gogo.proto";
import "google/api/annotations.proto";
import "cosmos/staking/v1beta1/staking.proto";
import "cosmos_proto/cosmos.proto";
import "cosmos/query/v1/query.proto";

option go_package = "github.com/cosmos/cosmos-sdk/x/staking/types";

// Query defines the gRPC querier service.
service Query {
// Validators queries all validators that match the given status.
rpc Validators(QueryValidatorsRequest) returns (QueryValidatorsResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/staking/v1beta1/validators";
}

// Validator queries validator info for given validator address.
rpc Validator(QueryValidatorRequest) returns (QueryValidatorResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/staking/v1beta1/validators/{validator_addr}";
}

// ValidatorDelegations queries delegate info for given validator.
rpc ValidatorDelegations(QueryValidatorDelegationsRequest) returns (QueryValidatorDelegationsResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/staking/v1beta1/validators/{validator_addr}/delegations";
}

// ValidatorUnbondingDelegations queries unbonding delegations of a validator.
rpc ValidatorUnbondingDelegations(QueryValidatorUnbondingDelegationsRequest)
returns (QueryValidatorUnbondingDelegationsResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/staking/v1beta1/validators/"
"{validator_addr}/unbonding_delegations";
}

// Delegation queries delegate info for given validator delegator pair.
rpc Delegation(QueryDelegationRequest) returns (QueryDelegationResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/staking/v1beta1/validators/{validator_addr}/delegations/"
"{delegator_addr}";
}

// UnbondingDelegation queries unbonding info for given validator delegator
// pair.
rpc UnbondingDelegation(QueryUnbondingDelegationRequest) returns (QueryUnbondingDelegationResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/staking/v1beta1/validators/{validator_addr}/delegations/"
"{delegator_addr}/unbonding_delegation";
}

// DelegatorDelegations queries all delegations of a given delegator address.
rpc DelegatorDelegations(QueryDelegatorDelegationsRequest) returns (QueryDelegatorDelegationsResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/staking/v1beta1/delegations/{delegator_addr}";
}

// DelegatorUnbondingDelegations queries all unbonding delegations of a given
// delegator address.
rpc DelegatorUnbondingDelegations(QueryDelegatorUnbondingDelegationsRequest)
returns (QueryDelegatorUnbondingDelegationsResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/staking/v1beta1/delegators/"
"{delegator_addr}/unbonding_delegations";
}

// Redelegations queries redelegations of given address.
rpc Redelegations(QueryRedelegationsRequest) returns (QueryRedelegationsResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/staking/v1beta1/delegators/{delegator_addr}/redelegations";
}

// DelegatorValidators queries all validators info for given delegator
// address.
rpc DelegatorValidators(QueryDelegatorValidatorsRequest) returns (QueryDelegatorValidatorsResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/staking/v1beta1/delegators/{delegator_addr}/validators";
}

// DelegatorValidator queries validator info for given delegator validator
// pair.
rpc DelegatorValidator(QueryDelegatorValidatorRequest) returns (QueryDelegatorValidatorResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/staking/v1beta1/delegators/{delegator_addr}/validators/"
"{validator_addr}";
}

// HistoricalInfo queries the historical info for given height.
rpc HistoricalInfo(QueryHistoricalInfoRequest) returns (QueryHistoricalInfoResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/staking/v1beta1/historical_info/{height}";
}

// Pool queries the pool info.
rpc Pool(QueryPoolRequest) returns (QueryPoolResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/staking/v1beta1/pool";
}

// Parameters queries the staking parameters.
rpc Params(QueryParamsRequest) returns (QueryParamsResponse) {
option (cosmos.query.v1.deterministic) = true;
option (google.api.http).get = "/cosmos/staking/v1beta1/params";
}
}
Expand Down