Mark wasm queries with module_query_safe #1915
Conversation
| rpc BuildAddress(QueryBuildAddressRequest) | ||
| returns (QueryBuildAddressResponse) { | ||
| option (cosmos.query.v1.module_query_safe) = true; |
There was a problem hiding this comment.
This query doesn't charge any gas. The option docs say:
- has its gas tracked, to avoid the attack vector where no gas is accounted for on potentially high-computation queries.
I guess this query is so computationally cheap that it's not an issue, but would be good to check somehow.
There was a problem hiding this comment.
good 👀 ! let's check what @webmaster128 thinks about this
There was a problem hiding this comment.
This is for creating Instantiate2 addresses, right?
I think this query should be removed as the algorithm is stateless and fixed. There are multiple implementations in Rust, TypeScript and Go. I don't see any reason to perform a query to the node for this.
There was a problem hiding this comment.
@webmaster128 should we remove this as part of next release?
I can create a separate pr for it
There was a problem hiding this comment.
Let's just create a ticket as this might open a can of worms looking at the code. IMO this is not relevant enough to delay the release. We should get CosmWasm 2.1 and other fixes in wasmd out as soon as possible.
There was a problem hiding this comment.
Would't it be easy to charge a small amount of gas here for the sake of consistency?
There was a problem hiding this comment.
yes, I think it makes sense to charge some gas for this. I will update the pr
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1915 +/- ##
==========================================
+ Coverage 54.87% 54.90% +0.02%
==========================================
Files 65 65
Lines 9775 9777 +2
==========================================
+ Hits 5364 5368 +4
+ Misses 3866 3865 -1
+ Partials 545 544 -1
|
x/wasm/keeper/querier.go
Outdated
| @@ -428,6 +431,11 @@ func (q GrpcQuerier) BuildAddress(c context.Context, req *types.QueryBuildAddres | |||
| if len(salt) == 0 { | |||
| return nil, status.Error(codes.InvalidArgument, "empty salt") | |||
| } | |||
|
|
|||
| ctx := sdk.UnwrapSDKContext(c) | |||
| costBuildAddress := DefaultGasCostBuildAddress * types.DefaultGasMultiplier | |||
There was a problem hiding this comment.
Is this multiplied by the gas multiplier on purpose? To me the code looks like it's just using the cosmos sdk gas meter, so that expects sdk gas as input, not cosmwasm gas.
There was a problem hiding this comment.
Yes, it makes sense to remove the Multiplier because it is related to CW gas.
I used it to make the cost in line with humanizeAddress operation.
I will update the pr and remove it. @chipshort do you think something like 500_000 is a good value for DefaultGasCostBuildAddress ?
x/wasm/keeper/querier.go
Outdated
| @@ -25,7 +25,7 @@ import ( | |||
| ) | |||
|
|
|||
| // DefaultGasCostBuildAddress is the SDK gas cost to build a contract address | |||
| const DefaultGasCostBuildAddress = 5 | |||
| const DefaultGasCostBuildAddress = 500_000 | |||
There was a problem hiding this comment.
This is a Cosmos SDK gas value where 100_000-200_00 is the gas range of a full transaction with verification, state changes, events etc. I think we need some much smaller value here.
What about 10?
There was a problem hiding this comment.
Sounds good! I will update the pr 👍
Resolves #1903