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

Problem: CosmWasm is not enabled on chain-main #780

Merged
merged 22 commits into from
Jun 16, 2022

Conversation

devashishdxt
Copy link
Collaborator

Solution: Integrated wasmd in app.go (have used temporary forks for ics29 fee middleware changes)

@codecov
Copy link

codecov bot commented May 30, 2022

Codecov Report

Merging #780 (f109494) into master (a7ac0c3) will increase coverage by 0.95%.
The diff coverage is 37.83%.

@@            Coverage Diff             @@
##           master     #780      +/-   ##
==========================================
+ Coverage   20.17%   21.13%   +0.95%     
==========================================
  Files          99       98       -1     
  Lines       11325    10813     -512     
==========================================
  Hits         2285     2285              
+ Misses       8553     8038     -515     
- Partials      487      490       +3     
Flag Coverage Δ
integration_tests 18.57% <48.27%> (+0.20%) ⬆️
unit_tests 7.22% <0.00%> (+0.35%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
app/test_helpers.go 0.00% <ø> (ø)
app/app.go 45.22% <36.84%> (+22.97%) ⬆️
cmd/chain-maind/app/app.go 53.84% <37.50%> (-0.11%) ⬇️
app/ante.go 61.29% <40.00%> (+16.55%) ⬆️
x/supply/keeper/genesis.go 6.02% <0.00%> (-1.82%) ⬇️
x/icaauth/types/params.pb.go 0.00% <0.00%> (-1.21%) ⬇️
app/encoding.go 100.00% <0.00%> (ø)
x/nft/simulation/operations.go 0.00% <0.00%> (ø)
app/genesis.go
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7ac0c3...f109494. Read the comment docs.

app/app.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@yihuang
Copy link
Collaborator

yihuang commented Jun 2, 2022

The windows build doesn't work because no prebuilt libwasmvm.dll in wasmvm repo.

Copy link
Collaborator

@yihuang yihuang left a comment

Choose a reason for hiding this comment

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

The nix building works now, can you add some basic integration test to show it works?

@devashishdxt
Copy link
Collaborator Author

/runsim

@github-actions
Copy link

Simulation tests started and triggered by /runsim.
Will update here when it succeeds or fails.
Can further check progress here

@devashishdxt devashishdxt marked this pull request as ready for review June 10, 2022 09:07
@devashishdxt devashishdxt requested a review from a team as a code owner June 10, 2022 09:07
@tomtau
Copy link
Contributor

tomtau commented Jun 10, 2022

Would it be possible to fix the Windows build? I think one can just add a cargo build step to this batch file: https://github.com/crypto-org-chain/chain-main/blob/master/makewin.bat to build wasmvm and have it on the path?

@github-actions
Copy link

/runsim simulation test has failed 😅
Can further check here

@devashishdxt
Copy link
Collaborator Author

/runsim

@github-actions
Copy link

Simulation tests started and triggered by /runsim.
Will update here when it succeeds or fails.
Can further check progress here

@devashishdxt
Copy link
Collaborator Author

This is their sample icaauth module. We already have wiring for that in app.go.

@github-actions
Copy link

/runsim simulation test has failed 😅
Can further check here

@devashishdxt
Copy link
Collaborator Author

Added

@devashishdxt
Copy link
Collaborator Author

Also, I guess the binary release via GoReleaser may not work any more? (at least, I assume it won't bundle the wasmvm dynamic library built by Rust by default)

As far as I understand this, pre-built dynamic libraries for linux and mac are linked by wasmvm. For windows, we may have to include .dll until CosmWasm/wasmvm#288 is merged and released. I've asked a question on discord for more details on this.

@github-actions
Copy link

/runsim simulation test has failed 😅
Can further check here

@yihuang
Copy link
Collaborator

yihuang commented Jun 13, 2022

Also, I guess the binary release via GoReleaser may not work any more? (at least, I assume it won't bundle the wasmvm dynamic library built by Rust by default)

As far as I understand this, pre-built dynamic libraries for linux and mac are linked by wasmvm. For windows, we may have to include .dll until CosmWasm/wasmvm#288 is merged and released. I've asked a question on discord for more details on this.

I guess it won't be packaged by goreleaser, and I've opened an issue that we might adopt the nix solution used in cronos: #781

@devashishdxt devashishdxt requested review from tomtau and yihuang June 14, 2022 04:27
@devashishdxt
Copy link
Collaborator Author

Would it be possible to fix the Windows build? I think one can just add a cargo build step to this batch file: https://github.com/crypto-org-chain/chain-main/blob/master/makewin.bat to build wasmvm and have it on the path?

I'll do this in next PR.

Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

updating the fork -- the windows build is currently marked as "Required" workflow to pass, so not sure if this can be merged without it being fixed

go.mod Outdated Show resolved Hide resolved
.gitmodules Show resolved Hide resolved
@yihuang
Copy link
Collaborator

yihuang commented Jun 14, 2022

/runsim simulation test has failed 😅 Can further check here

it failed, should we run again?

@devashishdxt
Copy link
Collaborator Author

/runsim simulation test has failed 😅 Can further check here

it failed, should we run again?

Yes. We can run again. It failed because it didn't find runsim binary.

@devashishdxt
Copy link
Collaborator Author

/runsim

@github-actions
Copy link

Simulation tests started and triggered by /runsim.
Will update here when it succeeds or fails.
Can further check progress here

@github-actions
Copy link

/runsim simulation test has succeeded 🎉
Can further check here

Copy link
Collaborator

@yihuang yihuang left a comment

Choose a reason for hiding this comment

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

LGTM

@devashishdxt devashishdxt requested a review from tomtau June 16, 2022 08:46
@tomtau tomtau merged commit a1dafca into crypto-org-chain:master Jun 16, 2022
@devashishdxt devashishdxt deleted the wasmd-integration branch June 17, 2022 00:39
devashishdxt added a commit to devashishdxt/chain-main that referenced this pull request Jul 15, 2022
… (fixes crypto-org-chain#735)

Solution: Integrated `wasmd` in `app.go` (have used temporary forks for ics29 fee middleware changes and windows fix)
devashishdxt added a commit that referenced this pull request Jul 15, 2022
Solution: Integrated `wasmd` in `app.go` (have used temporary forks for ics29 fee middleware changes and windows fix)
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