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

Add customizable WasmLimits #1989

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Add customizable WasmLimits #1989

wants to merge 11 commits into from

Conversation

chipshort
Copy link
Collaborator

closes #1986

Copy link

codecov bot commented Sep 15, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 48.64%. Comparing base (81c1eb0) to head (f79b23f).

Files with missing lines Patch % Lines
x/wasm/keeper/querier.go 0.00% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1989      +/-   ##
==========================================
- Coverage   48.66%   48.64%   -0.02%     
==========================================
  Files          65       65              
  Lines       10036    10040       +4     
==========================================
  Hits         4884     4884              
- Misses       4718     4722       +4     
  Partials      434      434              
Files with missing lines Coverage Δ
x/wasm/keeper/querier.go 76.39% <0.00%> (-1.02%) ⬇️

@chipshort
Copy link
Collaborator Author

obviously not mergeable yet, but would love some feedback from you @pinosu

Copy link
Contributor

@pinosu pinosu left a comment

Choose a reason for hiding this comment

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

Nice work!
I'm still thinking if it's best to have the wasmLimits in wasmConfig or add a new paramenter to the NewKeeper func . I guess we need to think about the scope of the two things and if they can match or just have similar names.

// QueryWasmLimitsConfigResponse is the response type for the
// Query/WasmLimitsConfig RPC method. It contains the JSON encoded limits for
// static validation of Wasm files.
message QueryWasmLimitsConfigResponse { string config = 1; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a specific struct that we can define for config or we want to keep it flexible?
In that case, we could evaluate using bytes instead of string. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As mentioned in our chat, I want to keep it flexible. The reason for string instead of bytes is that I want to make it easy for contract devs to copy and paste the limits. They need pass them into cosmwasm-check. See here: https://github.com/CosmWasm/cosmwasm/blob/cf413c5ad6a58a87e0be894584f01506f3b2e0af/packages/check/src/main.rs#L41-L50

x/wasm/keeper/test_common.go Outdated Show resolved Hide resolved
@@ -315,8 +315,14 @@ func NewWasmCoins(cosmosCoins sdk.Coins) (wasmCoins []wasmvmtypes.Coin) {
return wasmCoins
}

// WasmConfig is the extra config required for wasm
type WasmConfig struct {
// VMConfig contains configurations that are passed on to CosmWasm VM.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@pinosu pinosu left a comment

Choose a reason for hiding this comment

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

Great work!
Should we also define DefaultVMConfig(), with the limits set o some reasonable default value?

@chipshort
Copy link
Collaborator Author

Should we also define DefaultVMConfig(), with the limits set o some reasonable default value?

This is handled in cosmwasm-vm because I want to avoid chain developers forgetting to set one of the values to the default. That's why the values all use pointers. If you pass a nil ptr, cosmwasm-vm uses the default value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass Config to wasmvm and make WasmLimits available to contract dev
2 participants