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: QOL Capabilities functions #1316

Closed
wants to merge 1 commit into from
Closed

feat: QOL Capabilities functions #1316

wants to merge 1 commit into from

Conversation

Reecepbcups
Copy link

Per: CosmWasm/wasmvm#413

I have added some capability functions here instead of in wasmvm.

This way a chain like Juno can add wasmd.GetDefaultWasmCapabilities() for future upgrades without us forgetting to add the capabilities (that wasmd implement)

@Reecepbcups Reecepbcups requested a review from alpe as a code owner April 3, 2023 23:18
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #1316 (c24c67a) into main (b33c38c) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1316      +/-   ##
==========================================
- Coverage   63.47%   63.46%   -0.01%     
==========================================
  Files          56       56              
  Lines        6971     6975       +4     
==========================================
+ Hits         4425     4427       +2     
- Misses       2218     2219       +1     
- Partials      328      329       +1     
Impacted Files Coverage Δ
app/app.go 86.39% <100.00%> (+0.11%) ⬆️

... and 1 file with indirect coverage changes

@pinosu
Copy link
Contributor

pinosu commented Apr 6, 2023

Thanks for your pr!
Alex is on holidays and will be back in 2 weeks and he can give a better feedback on this work.
Regarding your changes, I am not sure if "default cosmos capabilities" and "default wasm capabilities" names are a bit misleading.
Moreover don't you lose visibility on which capabilities you are actually importing, and don't you need to check anyway on every version upgrade that those functions haven't been modified and some capability was added/removed?

@Reecepbcups
Copy link
Author

Reecepbcups commented Apr 6, 2023

@pinosu

I agree default can be misleading. Will change that worder

Moreover don't you lose visibility on which capabilities you are actually importing

I just wanted to solve for new wasmvm/wasmd functionality as it comes out. This way upgraded chains don't forget to add cosmwasm_1_# to their capabilities as we get more features.

That way as you upgrade (state breaking) to a future wasmvm 1.3 it would return capabilities cosmwasm_1_1, 1_2, and 1_3 automatically

This is all I was trying to solve for here as we forgot to add this during our testnet launch

@pinosu
Copy link
Contributor

pinosu commented Apr 7, 2023

Thank you @Reecepbcups !
I think the issue you want to solve makes sense, we just need to find the proper way to do it.
Let's continue this discussion when @alpe is back, he might have some good ideas about it.

@alpe
Copy link
Contributor

alpe commented Apr 17, 2023

Thanks for the PR! I do understand that new capabilities are easy to miss. Therefore I had highlighted this in the changelog.
Nevertheless I am very open to make this better.

With your solution, I have some concerns about the method names as well as the grouping. Let's spike some solutions that work nicely with git diff. I have opened #1341

@alpe alpe added the spike Demo to showcase an idea. label Apr 17, 2023
@alpe
Copy link
Contributor

alpe commented Apr 24, 2023

Closing in favour of #1361 . Thanks for bringing attention to the problem! 🏅

@alpe alpe closed this Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spike Demo to showcase an idea.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants