-
Notifications
You must be signed in to change notification settings - Fork 442
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
Apply review feedback #1376
Apply review feedback #1376
Conversation
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.
Good stuff. Why aren't they done inline in the PR itself? (I can review the diff or a commit or two)
I guess the e2e tests deserve their own PR. But fine to merge this and mark some comments as resolved.
@@ -133,7 +133,10 @@ func (k Keeper) OnRecvPacket( | |||
res, gasUsed, execErr := k.wasmVM.IBCPacketReceive(codeInfo.CodeHash, env, msg, prefixStore, cosmwasmAPI, querier, ctx.GasMeter(), gas, costJSONDeserialization) | |||
k.consumeRuntimeGas(ctx, gasUsed) | |||
if execErr != nil { | |||
panic(execErr) // let contract fully abort IBC receive in certain case | |||
panic(execErr) // let the contract fully abort an IBC packet receive. |
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 for the feedback!
I need to backport the changes to main. I hope this is easier with a separate PR. |
… (#1358) * Redesign IBC on packet recv error/ result.Err handling (cherry picked from commit 7cd5893) # Conflicts: # x/wasm/ibc.go # x/wasm/ibc_test.go # x/wasm/keeper/relay.go * Resolve conflicts * Apply review feedback (#1376) * Apply review feedback * Better test name * Add contract integration test * Better doc in test and minor refactoring --------- Co-authored-by: Alex Peters <alpe@users.noreply.github.com>
@Mergifyio backport main |
❌ No backport have been created
Git reported the following error:
|
@Mergifyio backport main |
✅ Backports have been created
|
* Apply review feedback * Better test name * Add contract integration test * Better doc in test and minor refactoring (cherry picked from commit 29481db)
* add proto files * Update proto/osmosis/tokenfactory/v1beta1/authorityMetadata.proto Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * update comments on proto * add other types files * Apply suggestions from code review Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * update errors to not start from 1 * add more comments * add logic * update app setup * add back query * Apply suggestions from code review Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * add migration * Apply suggestions from code review Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * address comment * Apply suggestions from code review Co-authored-by: Roman <roman@osmosis.team> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Roman <roman@osmosis.team> * address comments * removed mintTo and burnFrom * fixed typo * fix comments * metadata Validate() function * fix stuff * add mint restriction * remove unused argument from NewMsgBurn() * x/tokenfactory: cli * fix extra arguments * address comments * Apply suggestions from code review Co-authored-by: Roman <roman@osmosis.team> * ? * params * tests * add params command Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Roman <roman@osmosis.team>
Contains
Note: the ibc-reflect contract used is CosmWasm/cosmwasm#1685