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

feat: separate custom features in x/wasm into x/wasmplus module #7

Merged
merged 17 commits into from
Feb 6, 2023

Conversation

zemyblue
Copy link
Member

@zemyblue zemyblue commented Jan 30, 2023

Description

closes: #6

  • Separate custom features in x/wasmd into x/wasmplus module.
    • Change converted lbm proto files directory to x/wasmplus/types.
    • x/wasmplus module wraps tx, query apis and everything of x/wasm module.
  • Add separate appplus cli for x/wasmplus to avoid import cycle problem.
  • But I remove custom genesis params and custom query and tx msg encoding feature.

Why remove custom genesis params

The gas_multiplier, instance_cost and compile_cost of added custom genesis param are added to be enable to modify by proposal. And these params decide the gas fee when store and instantiate a smart contract.

However, we are using default values not change and we cannot control it without modifying the original source code.
So I think it's better to keep original logic, and change these const value and migrate if we need to modify this gas cost.

Why remove custom query and tx msg encoding feature

  • We do not use any more this feature.

Motivation and context

x/wasmd modules is origin wasmd module, but we did modify it. So we need to fix many conflicts when bump up original changes. So I think it's good to keep original x/wasmd module and separate the custom featues into x/wasmplus.

How has this been tested?

make build
make test

Screenshots (if appropriate):

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have updated API documentation client/docs/swagger-ui/swagger.yaml

@zemyblue zemyblue self-assigned this Jan 30, 2023
@zemyblue zemyblue marked this pull request as draft January 30, 2023 09:27
@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Merging #7 (0e91bf7) into main (51dd725) will increase coverage by 1.92%.
The diff coverage is 64.53%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main       #7      +/-   ##
==========================================
+ Coverage   58.88%   60.80%   +1.92%     
==========================================
  Files          63       80      +17     
  Lines        9002     9877     +875     
==========================================
+ Hits         5301     6006     +705     
- Misses       3396     3555     +159     
- Partials      305      316      +11     
Impacted Files Coverage Δ
appplus/test_helpers.go 0.00% <0.00%> (ø)
x/wasm/client/cli/gov_tx.go 6.19% <ø> (+0.81%) ⬆️
x/wasm/client/cli/query.go 85.00% <ø> (-1.50%) ⬇️
x/wasm/client/cli/tx.go 30.87% <ø> (+7.65%) ⬆️
x/wasm/handler.go 68.88% <ø> (+3.50%) ⬆️
x/wasm/keeper/contract_keeper.go 95.00% <ø> (-0.46%) ⬇️
x/wasm/keeper/genesis.go 87.64% <ø> (+11.16%) ⬆️
x/wasm/keeper/legacy_querier.go 66.36% <ø> (+8.42%) ⬆️
x/wasm/keeper/msg_server.go 25.00% <ø> (+3.70%) ⬆️
x/wasm/keeper/proposal_handler.go 61.20% <ø> (+1.84%) ⬆️
... and 40 more

Comment on lines +826 to +828
for acc := range maccPerms {
modAccAddrs[authtypes.NewModuleAddress(acc).String()] = true
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
Comment on lines +837 to +839
for acc := range maccPerms {
blockedAddrs[authtypes.NewModuleAddress(acc).String()] = !allowedReceivingModAcc()[acc]
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
Comment on lines +919 to +921
for k, v := range maccPerms {
dupMaccPerms[k] = v
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
Comment on lines +227 to +229
for _, v := range keys {
ms.MountStoreWithDB(v, sdk.StoreTypeIAVL, db)
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
Comment on lines +231 to +233
for _, v := range tkeys {
ms.MountStoreWithDB(v, sdk.StoreTypeTransient, db)
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
Comment on lines +236 to +238
for _, v := range memKeys {
ms.MountStoreWithDB(v, sdk.StoreTypeMemory, db)
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
Comment on lines +296 to +298
for acc := range maccPerms {
blockedAddrs[authtypes.NewModuleAddress(acc).String()] = true
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
@zemyblue zemyblue force-pushed the feat/separate_wasmplus2 branch from 3195a93 to 1eb8763 Compare January 30, 2023 14:18
@zemyblue zemyblue changed the title feat: separate custom features in x/wasmd into x/wasmplus module feat: separate custom features in x/wasm into x/wasmplus module Jan 31, 2023
@zemyblue zemyblue marked this pull request as ready for review January 31, 2023 06:15
@shiki-tak
Copy link

shiki-tak commented Feb 2, 2023

When I run the command with the built binary, I get the following message, is this OK?

@mHXCV6H67LX build % ./wasmplusd version
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.crypto.multisig.v1beta1.MultiSignature
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.crypto.multisig.v1beta1.CompactBitArray
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.kv.v1beta1.Pairs
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.kv.v1beta1.Pair
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.snapshots.v1beta1.Snapshot
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.snapshots.v1beta1.Metadata
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.store.v1beta1.CommitInfo
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.store.v1beta1.StoreInfo
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.store.v1beta1.CommitID
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.store.v1beta1.StoreKVPair
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.TxResponse
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.ABCIMessageLog
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.StringEvent
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.Attribute
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.GasInfo
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.Result
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.SimulationResponse
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.MsgData
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.TxMsgData
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.SearchTxsResult
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.v1beta1.Coin
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.v1beta1.DecCoin
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.v1beta1.IntProto
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.v1beta1.DecProto

@zemyblue
Copy link
Member Author

zemyblue commented Feb 2, 2023

When I run the command with the built binary, I get the following message, is this OK?

@mHXCV6H67LX build % ./wasmplusd version
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.crypto.multisig.v1beta1.MultiSignature
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.crypto.multisig.v1beta1.CompactBitArray
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.kv.v1beta1.Pairs
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.kv.v1beta1.Pair
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.snapshots.v1beta1.Snapshot
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.snapshots.v1beta1.Metadata
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.store.v1beta1.CommitInfo
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.store.v1beta1.StoreInfo
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.store.v1beta1.CommitID
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.store.v1beta1.StoreKVPair
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.TxResponse
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.ABCIMessageLog
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.StringEvent
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.Attribute
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.GasInfo
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.Result
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.SimulationResponse
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.MsgData
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.TxMsgData
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.abci.v1beta1.SearchTxsResult
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.v1beta1.Coin
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.v1beta1.DecCoin
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.v1beta1.IntProto
2023/02/02 18:12:53 proto: duplicate proto type registered: cosmos.base.v1beta1.DecProto

This duplicate registery is a problem. But this problem need to fix in lbm-sdk.
This problem is occurred because lbm-sdk has same cosmos proto files.

I added this issue as Finschia/finschia-sdk#877.

@tkxkd0159
Copy link
Member

I have some questions

  1. Why do we need to implement appplus and wasmplusd? I think it's not sustainable. We have to track app, wasmd changes and apply them to our new implementation every time. I think we should resolve cycle problem with another method.
  2. lbm-sdk have duplicated modules against cosmos-sdk. So we assume that we only use lbm-sdk for our product. That means we shouldn't remove our cosmos proto unless we extract all modules which come from cosmos-sdk. The problem of duplicate proto comes from app logic which include interchain-accounts lib which we don't actually need. Therefore, I think it's better to just focus on unit tests for wasm module in this repo and do integration test on lbm.

@zemyblue
Copy link
Member Author

zemyblue commented Feb 3, 2023

I have some questions

  1. Why do we need to implement appplus and wasmplusd? I think it's not sustainable. We have to track app, wasmd changes and apply them to our new implementation every time. I think we should resolve cycle problem with another method.

I want to keep the codes of original cosmwasm/wasmd without changes. But your opinion is also good. I'll remove wasmplusd and replace wasmd for x/wasmplus. Thank you.

  1. lbm-sdk have duplicated modules against cosmos-sdk. So we assume that we only use lbm-sdk for our product. That means we shouldn't remove our cosmos proto unless we extract all modules which come from cosmos-sdk. The problem of duplicate proto comes from app logic which include interchain-accounts lib which we don't actually need. Therefore, I think it's better to just focus on unit tests for wasm module in this repo and do integration test on lbm.

Oh, I'll check it. Good.

@shiki-tak
Copy link

Is it possible to change the Usage of wasmplusd from wasmd to wasmplusd while still coexisting with the wasmd app?

Usage:
  wasmd query wasm [flags]
  wasmd query wasm [command]

appplus/app_test.go Show resolved Hide resolved
appplus/app_test.go Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
appplus/app_test.go Outdated Show resolved Hide resolved
appplus/app.go Outdated Show resolved Hide resolved
@zemyblue
Copy link
Member Author

zemyblue commented Feb 3, 2023

I have some questions

  1. Why do we need to implement appplus and wasmplusd? I think it's not sustainable. We have to track app, wasmd changes and apply them to our new implementation every time. I think we should resolve cycle problem with another method.

I want to keep the codes of original cosmwasm/wasmd without changes. But your opinion is also good. I'll remove wasmplusd and replace wasmd for x/wasmplus. Thank you.

When I check again, some unittest of x/wasm use app simulator and unittest library, so if we change the usage of app for x/wasmplus import cycle problem is occurred. So I made new appplus for x/wasmplus. But these wasmplusd cli will not be need if intergrated binary is exist and change public. I'll remove it later if integrated binary is public.

  1. lbm-sdk have duplicated modules against cosmos-sdk. So we assume that we only use lbm-sdk for our product. That means we shouldn't remove our cosmos proto unless we extract all modules which come from cosmos-sdk. The problem of duplicate proto comes from app logic which include interchain-accounts lib which we don't actually need. Therefore, I think it's better to just focus on unit tests for wasm module in this repo and do integration test on lbm.

Oh, I'll check it. Good.

appplus/app.go Outdated Show resolved Hide resolved
proto/lbm/wasm/v1/genesis.proto Outdated Show resolved Hide resolved
@zemyblue zemyblue requested a review from shiki-tak February 3, 2023 06:13
@shiki-tak
Copy link

When you update the proto file, please also update the proto-gen.

x/wasm/client/cli/gov_tx.go Outdated Show resolved Hide resolved
x/wasmplus/keeper/metrics.go Outdated Show resolved Hide resolved
x/wasmplus/keeper/keeper_extension.go Outdated Show resolved Hide resolved
 - move all function of `wasmplus/keeper/keeper_extension.go` to `wasmplus/keeper/keeper.go`
 - remove no need files (keeper_extension.go, metrics.go)
@zemyblue zemyblue requested a review from loloicci February 6, 2023 04:58
Copy link

@shiki-tak shiki-tak left a comment

Choose a reason for hiding this comment

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

LGTM

@zemyblue zemyblue merged commit 60fec8a into Finschia:main Feb 6, 2023
@zemyblue zemyblue mentioned this pull request Mar 28, 2023
@zemyblue zemyblue deleted the feat/separate_wasmplus2 branch April 2, 2023 07:40
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.

detach custom genesis and types as lbm proto
4 participants