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

Start Pin/Unpin contract #436

Merged
merged 1 commit into from
Mar 9, 2021
Merged

Start Pin/Unpin contract #436

merged 1 commit into from
Mar 9, 2021

Conversation

alpe
Copy link
Contributor

@alpe alpe commented Mar 4, 2021

Resolves #401

  • Add Pin/Unpin/IsPinned methods
  • Persist pinned code id state as second index
  • Export/ Import pinned flag via genesis
  • Extend engineMock
  • Gov proposal types
  • Benchmarks

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Nice start, a few comments

app/app.go Outdated Show resolved Hide resolved
x/wasm/internal/keeper/genesis.go Outdated Show resolved Hide resolved
x/wasm/internal/types/wasmer_engine.go Show resolved Hide resolved
x/wasm/internal/keeper/keeper.go Outdated Show resolved Hide resolved
x/wasm/internal/keeper/keeper.go Outdated Show resolved Hide resolved
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Nice stuff.

Still missing the pieces you noted above, but only a few stylistic nits in the code.

x/wasm/internal/keeper/keeper.go Show resolved Hide resolved
x/wasm/internal/keeper/keeper.go Outdated Show resolved Hide resolved
x/wasm/internal/keeper/keeper.go Show resolved Hide resolved
x/wasm/internal/keeper/keeper.go Outdated Show resolved Hide resolved
x/wasm/internal/types/keys.go Outdated Show resolved Hide resolved
@alpe alpe force-pushed the contract_pinning_401 branch from 93a20e7 to 6c3c197 Compare March 7, 2021 12:42
@CosmWasm CosmWasm deleted a comment from codecov bot Mar 7, 2021
@alpe alpe marked this pull request as ready for review March 7, 2021 12:42
@codecov
Copy link

codecov bot commented Mar 7, 2021

Codecov Report

Merging #436 (6c3c197) into master (d51a1b8) will increase coverage by 0.25%.
The diff coverage is 66.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
+ Coverage   55.44%   55.69%   +0.25%     
==========================================
  Files          39       39              
  Lines        4080     4189     +109     
==========================================
+ Hits         2262     2333      +71     
- Misses       1627     1663      +36     
- Partials      191      193       +2     
Impacted Files Coverage Δ
app/app.go 87.05% <0.00%> (-0.69%) ⬇️
x/wasm/internal/types/keys.go 38.23% <0.00%> (-8.20%) ⬇️
x/wasm/internal/types/proposal.go 82.72% <43.75%> (-6.64%) ⬇️
x/wasm/internal/keeper/genesis.go 87.87% <50.00%> (-2.45%) ⬇️
x/wasm/internal/types/codec.go 42.10% <50.00%> (+0.92%) ⬆️
x/wasm/internal/keeper/keeper.go 86.89% <75.60%> (-1.04%) ⬇️
x/wasm/internal/keeper/proposal_handler.go 66.91% <100.00%> (+9.36%) ⬆️

Copy link
Member

@ethanfrey ethanfrey 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.

I added one minor comment you may want to integrate later (testing.TB), but this looks great. I'm going to merge it in and start rebasing other PRs on top.

"testing"
)

func BenchmarkExecution(b *testing.B) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd love to see the numbers from this on, say, wasmd 0.12 (which had the old wasmer engine), so we can compare the speedup for uncached over the last 3-4 months, plus the huge speedup for pinned contracts

if err := p.ValidateBasic(); err != nil {
return err
}
for _, v := range p.CodeIDs {
Copy link
Member

Choose a reason for hiding this comment

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

nice, i like the list of code ids

}
require.NoError(t, gotErr)

// and proposal execute
Copy link
Member

Choose a reason for hiding this comment

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

nice shortcut to mock out the whole voting process

@@ -388,12 +405,12 @@ func handleExecute(ctx sdk.Context, k *Keeper, msg *types.MsgExecuteContract) (*
return res, nil
}

func RandomAccountAddress(_ *testing.T) sdk.AccAddress {
func RandomAccountAddress(_ TestingT) sdk.AccAddress {
Copy link
Member

Choose a reason for hiding this comment

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

There is a testing.TB interface I have seen before that probably is what you want here - one interface that lets you use the code both in unit tests as well as benchmarks.

@ethanfrey ethanfrey merged commit 4072abf into master Mar 9, 2021
@ethanfrey ethanfrey deleted the contract_pinning_401 branch March 9, 2021 21:49
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.

Expose contract pinning
2 participants