-
Notifications
You must be signed in to change notification settings - Fork 402
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
Mark wasm queries with module_query_safe #1915
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,9 @@ import ( | |
"github.com/CosmWasm/wasmd/x/wasm/types" | ||
) | ||
|
||
// DefaultGasCostBuildAddress is the SDK gas cost to build a contract address | ||
const DefaultGasCostBuildAddress = 500_000 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good! I will update the pr 👍 |
||
|
||
var _ types.QueryServer = &GrpcQuerier{} | ||
|
||
type GrpcQuerier struct { | ||
|
@@ -428,6 +431,10 @@ 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) | ||
defer ctx.GasMeter().ConsumeGas(DefaultGasCostBuildAddress, "build address") | ||
|
||
if req.InitArgs == nil { | ||
return &types.QueryBuildAddressResponse{ | ||
Address: BuildContractAddressPredictable(codeHash, creator, salt, []byte{}).String(), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This query doesn't charge any gas. The option docs say:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good 👀 ! let's check what @webmaster128 thinks about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I think it makes sense to charge some gas for this. I will update the pr