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

Make MaxLabelSize a var not const #822

Merged
merged 3 commits into from
Apr 27, 2022
Merged

Make MaxLabelSize a var not const #822

merged 3 commits into from
Apr 27, 2022

Conversation

ethanfrey
Copy link
Member

Addresses #813 along with #809

MaxLabelSize is a now var, which can be overridden compile-time by any chain that imports x/wasm.
The 128 char limit, while reasonable, was an arbitrary choice by Confio developers and a blockchain may wish to customize it.

To override, set this variable to a new value in some initialisation code (either app.go or cmd/../main.go)

@ethanfrey ethanfrey requested a review from alpe as a code owner April 27, 2022 13:42
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.

lgtm 👍

@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #822 (3a9a851) into main (7a25e36) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #822      +/-   ##
==========================================
- Coverage   59.29%   59.26%   -0.04%     
==========================================
  Files          50       50              
  Lines        5884     5884              
==========================================
- Hits         3489     3487       -2     
- Misses       2143     2144       +1     
- Partials      252      253       +1     
Impacted Files Coverage Δ
x/wasm/types/validation.go 100.00% <ø> (ø)
x/wasm/keeper/keeper.go 87.47% <0.00%> (-0.35%) ⬇️

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Bonus points, if you could add some doc to
https://github.com/CosmWasm/wasmd#runtime-flags
for this new flag

@ethanfrey
Copy link
Member Author

I did that and then tested like:

go test -count=1 -ldflags '-X github.com/CosmWasm/wasmd/x/wasm/types.MaxLabelSize=64' ./keeper

But that fails with:

github.com/CosmWasm/wasmd/x/wasm/types.MaxLabelSize: cannot set with -X: not a var of type string (type.int)

I would try to make this a string and parse it and all, but I think it is easier to just say to override in app.go. People modifying this should have their own repo importing it anyway

@ethanfrey ethanfrey merged commit 8ca55b7 into main Apr 27, 2022
@ethanfrey ethanfrey deleted the move-const-label-to-var branch April 27, 2022 19:01
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.

4 participants