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

Apply review feedback #1376

Merged
merged 4 commits into from
May 8, 2023
Merged

Conversation

alpe
Copy link
Contributor

@alpe alpe commented May 3, 2023

Contains

  • Cleanup
  • Better comments
  • Integration tests with real wasm contract

Note: the ibc-reflect contract used is CosmWasm/cosmwasm#1685

Copy link
Member

@ethanfrey ethanfrey left a 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.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@alpe alpe added the backport/main Backport patches to main branch label May 5, 2023
@alpe
Copy link
Contributor Author

alpe commented May 5, 2023

Thanks for the feedback!

Why aren't they done inline in the PR itself?

I need to backport the changes to main. I hope this is easier with a separate PR.

@alpe alpe marked this pull request as ready for review May 8, 2023 15:20
@alpe alpe requested review from pinosu and removed request for pinosu May 8, 2023 15:20
@alpe alpe merged commit 29481db into mergify/bp/releases/v0.3x/pr-1353 May 8, 2023
@alpe alpe deleted the 1358_review_updates branch May 8, 2023 15:21
alpe added a commit that referenced this pull request May 10, 2023
… (#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>
@alpe
Copy link
Contributor Author

alpe commented May 10, 2023

@Mergifyio backport main

@mergify
Copy link
Contributor

mergify bot commented May 10, 2023

backport main

❌ No backport have been created

  • Backport to branch main failed

Git reported the following error:

fatal: couldn't find remote ref mergify/bp/releases/v0.3x/pr-1353

@alpe
Copy link
Contributor Author

alpe commented May 10, 2023

@Mergifyio backport main

@mergify
Copy link
Contributor

mergify bot commented May 10, 2023

backport main

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 10, 2023
* Apply review feedback

* Better test name

* Add contract integration test

* Better doc in test and minor refactoring

(cherry picked from commit 29481db)
alpe added a commit that referenced this pull request May 10, 2023
* Apply review feedback (#1376)

* Apply review feedback

* Better test name

* Add contract integration test

* Better doc in test and minor refactoring

(cherry picked from commit 29481db)

* Set correct test chain id

---------

Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>
larry0x pushed a commit to larry0x/wasmd that referenced this pull request Jul 17, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/main Backport patches to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants