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.

Sign up for GitHub

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

Call Pin during app inisitalization #5161

Merged
merged 15 commits into from
Nov 28, 2023
+118 −8
Merged

Call Pin during app inisitalization #5161

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3344e8d
init InitializePinnedCodes
vuong177 Nov 22, 2023
71c6253
testing
vuong177 Nov 22, 2023
ad26127
call Pin during app inisitalization
vuong177 Nov 22, 2023
f362bac
linting
vuong177 Nov 22, 2023
0ed221f
Merge branch 'main' into vuong/pin-during-appinit
vuong177 Nov 22, 2023
ef2d6d8
update docs
Nov 23, 2023
5382516
spacing
Nov 23, 2023
c9c4d11
update docs
Nov 23, 2023
7b73fce
Merge branch 'main' into vuong/pin-during-appinit
Nov 23, 2023
43a7371
add extra line
Nov 26, 2023
a97ab91
chore: update tmproto, tmos aliases to cmtproto, cmtos
damiannolan Nov 27, 2023
d639b49
chore: use suite.Require().NoError(), rename gotErr to err
damiannolan Nov 27, 2023
f7aa8f7
testing: nits
damiannolan Nov 27, 2023
c6cdffb
Use table driven tests for TestInitializedPinnedCodes
DimitrisJim Nov 28, 2023
4545911
Merge branch 'main' into vuong/pin-during-appinit
DimitrisJim Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions modules/light-clients/08-wasm/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,3 +188,18 @@ func (k Keeper) GetWasmClientState(ctx sdk.Context, clientID string) (*types.Cli

return wasmClientState, nil
}

// InitializePinnedCodes updates wasmvm to pin to cache all contracts marked as pinned
func InitializePinnedCodes(ctx sdk.Context) error {
checksums, err := types.GetAllChecksums(ctx)
if err != nil {
return err
}

for _, checksum := range checksums {
if err := ibcwasm.GetVM().Pin(checksum); err != nil {
return err
}
}
return nil
}
37 changes: 37 additions & 0 deletions modules/light-clients/08-wasm/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,3 +229,40 @@ func (suite *KeeperTestSuite) TestNewKeeper() {
})
}
}

func (suite *KeeperTestSuite) TestInitializePinnedCodes() {
suite.SetupWasmWithMockVM()
var capturedChecksums []wasmvm.Checksum

suite.mockVM.PinFn = func(checksum wasmvm.Checksum) error {
capturedChecksums = append(capturedChecksums, checksum)
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

for another time: this might be nice as a default behavior for mockVM (pin appends, unpin pops). That way we should also be able to more granularly check these


gzippedContract, err := types.GzipIt(append(wasmtesting.WasmMagicNumber, []byte("gzipped-contract")...))
Copy link
Contributor

Choose a reason for hiding this comment

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

#5162 might be available too if it gets merged before this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it

suite.Require().NoError(err)

ctx := suite.chainA.GetContext().WithBlockGasMeter(storetypes.NewInfiniteGasMeter())

contracts := [][]byte{wasmtesting.Code, gzippedContract}
checksumIDs := make([]types.Checksum, len(contracts))

wasmClientKeeper := GetSimApp(suite.chainA).WasmClientKeeper
Copy link
Contributor

Choose a reason for hiding this comment

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

we should require an updated gas meter here. Also, logical grouping of assignments nit:

Suggested change
ctx := suite.chainA.GetContext().WithBlockGasMeter(storetypes.NewInfiniteGasMeter())
contracts := [][]byte{wasmtesting.Code, gzippedContract}
checksumIDs := make([]types.Checksum, len(contracts))
wasmClientKeeper := GetSimApp(suite.chainA).WasmClientKeeper
ctx := suite.chainA.GetContext()
wasmClientKeeper := GetSimApp(suite.chainA).WasmClientKeeper
contracts := [][]byte{wasmtesting.Code, gzippedContract}
checksumIDs := make([]types.Checksum, len(contracts))

// store contract on chain
for i, contract := range contracts {
signer := authtypes.NewModuleAddress(govtypes.ModuleName).String()
Copy link
Contributor

@DimitrisJim DimitrisJim Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

can also move signer to top of func with captured checksums since we don't need to call it in a loop

msg := types.NewMsgStoreCode(signer, contract)

res, err := wasmClientKeeper.StoreCode(ctx, msg)
suite.Require().NoError(err)

checksumIDs[i] = res.Checksum
}

capturedChecksums = nil

gotErr := keeper.InitializePinnedCodes(ctx)
suite.NoError(gotErr)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved

suite.ElementsMatch(checksumIDs, capturedChecksums)
}
9 changes: 9 additions & 0 deletions modules/light-clients/08-wasm/testing/simapp/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ import (
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"

abci "github.com/cometbft/cometbft/abci/types"
tmos "github.com/cometbft/cometbft/libs/os"
tmproto "github.com/cometbft/cometbft/proto/tendermint/types"
Copy link
Contributor

Choose a reason for hiding this comment

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

we're renaming these in #5158

Can we change to cmos and cmproto?

Copy link
Contributor

Choose a reason for hiding this comment

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

damn, there's also a tmos? Hadn't found that in my grepping.


"github.com/cosmos/ibc-go/modules/capability"
capabilitykeeper "github.com/cosmos/ibc-go/modules/capability/keeper"
Expand Down Expand Up @@ -815,6 +817,13 @@ func NewSimApp(
if err := app.LoadLatestVersion(); err != nil {
panic(fmt.Errorf("error loading last version: %w", err))
}

ctx := app.BaseApp.NewUncachedContext(true, tmproto.Header{})

// Initialize pinned codes in wasmvm as they are not persisted there
if err := wasmkeeper.InitializePinnedCodes(ctx); err != nil {
tmos.Exit(fmt.Sprintf("failed initialize pinned codes %s", err))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I pushed a commit updating the documentation to add this. Thanks, @vuong177!

}

app.ScopedIBCKeeper = scopedIBCKeeper
Expand Down
Loading