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!: cosmwasm an optional integration to app #1547

Merged
merged 16 commits into from
Nov 8, 2022
Merged

Conversation

gsk967
Copy link
Collaborator

@gsk967 gsk967 commented Nov 4, 2022

Description

added cosmwasm feature as an optional features for app

// to build cosmwasm feature into app 
$ EXPERIMENTAL=true make build

closes: #802


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • added appropriate labels to the PR
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@gsk967 gsk967 changed the title feat!: cosmwasm optional integration to app feat!: cosmwasm an optional integration to app Nov 4, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2022

Codecov Report

Merging #1547 (7e4405d) into main (2791f00) will decrease coverage by 0.19%.
The diff coverage is 3.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1547      +/-   ##
==========================================
- Coverage   51.79%   51.59%   -0.20%     
==========================================
  Files          71       71              
  Lines        6897     6923      +26     
==========================================
  Hits         3572     3572              
- Misses       3056     3081      +25     
- Partials      269      270       +1     
Impacted Files Coverage Δ
ante/ante.go 32.14% <3.70%> (-27.86%) ⬇️

app/app.go Fixed Show fixed Hide fixed
all modules verified
--> Installing...
go install -mod=readonly -tags "netgo ledger" -ldflags '-X github.com/cosmos/cosmos-sdk/version.Name=umee -X github.com/cosmos/cosmos-sdk/version.AppName=umeed -X github.com/cosmos/cosmos-sdk/version.Version=sai/cosmwasm-c1f6f1f3e9322bf2c110a999dcc42bc07e6d673a -X github.com/cosmos/cosmos-sdk/version.Commit=c1f6f1f3e9322bf2c110a999dcc42bc07e6d673a -X "github.com/cosmos/cosmos-sdk/version.BuildTags=netgo,ledger" -X github.com/tendermint/tendermint/version.TMCoreSemVer=v0.34.22 -X github.com/umee-network/umee/v3/x/leverage/keeper.EnableLiquidator=false' ./...
@gsk967 gsk967 marked this pull request as ready for review November 4, 2022 12:04
@gsk967 gsk967 requested review from a team as code owners November 4, 2022 12:04
Copy link
Member

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Left few comments.
Also, have an idea how to better integrate the experimental build without doing so many custom functions. I will make a commit with an example directly in your repo, let me know what do you think.

.goreleaser.yml Show resolved Hide resolved
app/experimental_appconfig.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
app/experimental_appconfig.go Outdated Show resolved Hide resolved
app/experimental_appconfig.go Outdated Show resolved Hide resolved
app/stable_appconfig.go Outdated Show resolved Hide resolved
app/stable_appconfig.go Outdated Show resolved Hide resolved
@robert-zaremba
Copy link
Member

See this commit as an alternative idea to reduce copied functions: 009f87d
What do you think?

@gsk967
Copy link
Collaborator Author

gsk967 commented Nov 6, 2022

See this commit as an alternative idea to reduce copied functions: 009f87d What do you think?

I think it is better to keep old functions only, because we already passing one build flag for experimental config , so no need for new flag again.
We will keep experimental features everything into experimenatal_appconfig only.

app/app.go Fixed Show fixed Hide fixed
@robert-zaremba
Copy link
Member

robert-zaremba commented Nov 7, 2022

I think it is better to keep old functions only, because we already passing one build flag for experimental config , so no need for new flag again.

There is no need for a new flag. Where did you get an impression that we need a new flag?

We will keep experimental features everything into experimenatal_appconfig only.

On the other hand, the code is a bit more bloated. So my idea was to use both: "if else" when the change is small, and function in other places. Especially the maccPerms looks much cleaner with the "if else" approach.

@gsk967 gsk967 requested a review from robert-zaremba November 8, 2022 16:37
@gsk967
Copy link
Collaborator Author

gsk967 commented Nov 8, 2022

@robert-zaremba Please can you review again

Copy link
Member

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Looks good, let's solve the remaining comments.

umee.e2e.Dockerfile Show resolved Hide resolved
@robert-zaremba robert-zaremba merged commit 54c0c1f into main Nov 8, 2022
@robert-zaremba robert-zaremba deleted the sai/cosmwasm branch November 8, 2022 23:56
@adamewozniak adamewozniak mentioned this pull request Nov 15, 2022
17 tasks
@robert-zaremba robert-zaremba mentioned this pull request Dec 21, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add CosmWasm
3 participants