-
Notifications
You must be signed in to change notification settings - Fork 658
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!: integrate cosmwasm #3051
base: main
Are you sure you want to change the base?
Conversation
app/upgrades/v16/upgrades.go
Outdated
wasmParams.CodeUploadAccess = wasmtypes.AllowNobody | ||
// TODO(reece): only allow specific addresses to instantiate contracts or anyone with AccessTypeEverybody? | ||
wasmParams.InstantiateDefaultPermission = wasmtypes.AccessTypeAnyOfAddresses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need input on the team for if this. Should it be
InstantiateDefaultPermission = AccessTypeAnyOfAddresses
(whitelisted wallets can instantiate new contracts as much as they want off of approve code ids from gov)- or
InstantiateDefaultPermission = AccessTypeNobody
(only gov only can create contracts off previously gov approved code)
To me AccessTypeAnyOfAddresses
makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want the governance to handle both code upload and instantiation.
IIUC, to use permissiond wasm we would need to use these settings:
wasmParams := wasmtypes.Params{
CodeUploadAccess: wasmtypes.AllowNobody,
InstantiateDefaultPermission: wasmtypes.AllowNobody,
}
I don't think that code upload is followed by an instantiation operation by default. But, that can be can be achieved through a gov prop that has 2 Txs to execute: [MsgStoreCode, MsgInstantiateContract]
. This way, the uploaded contracts would immediately get instantiated.
We can use MsgStoreAndInstantiateContract
to store and instantiate a contract via gov proposal.
Do you have any comments on this @Reecepbcups ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes total sense yes, wasmtypes.AllowNobody is the desired param for both
MsgStoreAndInstantiateContract looks like it will allow the uploader (gov) to override the AccessConfig though for instantiates. InstantiateContractProposal
does not have this.
I would assume this overrides the defaults since gov approved, which is a feature but actually a bug in our case for expected behavior. i.e. need to ante block a proposal if MsgStoreAndInstantiateContract.instantiate_permission
is set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm getting it, thanks!
We will address those concerns before testnet and write new docs before cosmwasm lands on the hub mainnet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff 🎉 I'm sure you know the Go integration part better than myself
@@ -20,10 +20,8 @@ builds: | |||
goos: | |||
- darwin | |||
- linux | |||
- windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Windows we can still build a client binary (through some build flags). However, the contract execution on Windows is not supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if goreleaser lets you specify the GOOS=windows GOARCH=amd64 go build $(BUILD_FLAGS) -o build/gaiad.exe ./cmd/gaiad
for just 1 type of arch. Will have to look more into
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can take a look at this but I think you're right about the env vars.
Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>
sed -i '' 's/timeout_commit = "5s"/timeout_commit = "1s"/g' ~/.gaia/config/config.toml | ||
sed -i '' 's/timeout_propose = "3s"/timeout_propose = "1s"/g' ~/.gaia/config/config.toml | ||
sed -i '' 's/index_all_keys = false/index_all_keys = true/g' ~/.gaia/config/config.toml | ||
# sed -i '' 's#"tcp://127.0.0.1:26657"#"tcp://0.0.0.0:26657"#g' ~/.gaia/config/config.toml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These scripts probably need to be redone tbh. Happy to do if the team would like. Maybe another PR
ref what I use: https://github.com/CosmosContracts/juno/blob/main/scripts/test_node.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have this in the backlob but we haven't started refactoring yet: #2950
We can add refactoring this script to that issue and do them all in a different PR.
@Reecepbcups I'd like to help pass all the checks (upgrade tests, e2e etc.) if you don't mind me adding to the PR? As for merging this, the v16 release is already feature frozen and will be cut tomorrow morning (European time) as we have already prepared all the testing pipelines and made plans for the testnet upgrades. It's a pretty beefy release adding IBC features voted in by governance. I think it should be pretty safe to merge this into main once we finalize the v16 release branch. I'm really excited about this 🚀 |
@MSalopek Thanks and Sounds good on the v16 front. Yes, Maintainer edits are enabled. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I’ll pull in main Tuesday in prep for v18* |
Conflicts resolved. Upgrade moved to gaia/v18
I already merged main into the branch (maintainer edits are enabled on the v17 is already cut and the propsal is in voting (it has partial set security features enabled with some other goodies). v18 is next with cosmwasm, ibc-go v7.5.0 and some other upgrades. The PR will be merged as soon as all tests are passing and we provide a definite answer regarding this discussion: #3051 (comment). The relatively fast upgrade cadence enables us to push out more features in distinct releases so we offset some risks around the upgrade. |
ENV PACKAGES curl make git libc-dev bash gcc linux-headers eudev-dev python3 | ||
RUN apk add --no-cache $PACKAGES | ||
RUN CGO_ENABLED=0 make install | ||
RUN LEDGER_ENABLED=false LINK_STATICALLY=true BUILD_TAGS=muslc make build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had issues with this in e2e.
CGO needs to be enabled (don't know why).
I've also had to add RUN apk add --no-cache build-base
in the runner container to make CGO available.
It seems that we cannot run the tests without CGO enabled.
I'm not sure what the implications of this are. While checking other projects, they all seem to be running with CGO enabled (or at least they don't explicitly disable it using CGO_ENABLED=0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This repo contains both Rust and Go code. The Rust code is compiled into a library (shared .dll/.dylib/.so or static .a) to be linked via cgo and wrapped with a pleasant Go API
CGO is a requirement for CW yes https://github.com/CosmWasm/wasmvm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the PR!
All tests were cleaned up, the upgrade is passing.
There's a bit of work to do later.
We still need to check the goreleaser settings, update Dockerfile and make better upgrade tests.
The build got a bit complicated given the libwasmvm
dep that can be either statically or dynamically linked. upgrade-test
is using a dynamic linking approach while dockerized build uses static linking.
We will need to expand the build docs to show how to build with cosmwasm in mind.
ref: #3050
Adds CosmWasm and makes it permissioned in the upgrade handler per proposal 895 signaling to add.
Test Local node Script
Upload Wasm Contract
Local Upgrade
TODO: