After the upgrades to the PFM to accommodate ibc-go v8 we are seeing one of the test cases fail in CI

see: https://github.com/cosmos/ibc-apps/actions/runs/7454152262/job/20284768753?pr=158

Hello,
I think it would be highly beneficial to make the packet-forward-middleware function getReceiver public

Thanks for your consideration.

I envision the badge being a part of the List of Apps table on the home readme.

Docs on create custom github badges:

• https://docs.github.com/en/enterprise-cloud@latest/apps/creating-github-apps/registering-a-github-app/creating-a-custom-badge-for-your-github-app
• https://gist.github.com/afig/be5ab20c50062dba7cb835e30206659a

Chains can then add the badge to their repository.

Maybe not the most elegant, but this seems like an easy win to allow chains to advertise their use of specific apps.

Example:

Let's say I want to query GetCmdEstimateSwapExactAmountIn from Osmosis, if you're querying a stableswap pool and enter more tokens than there are in a pool it will panic Here this will not cause the query to panic and not return an error thus async-icq will not return an ack-err.

We are missing some of the supported branches for async-icq. Bring those over and create tags

The current repository only contains the async-icq module and has a branching structure as shown in the image.

If we add more modules, it could become messy without a better branching naming convention. For example, if we add a new module called ibc-hooks and want to support multiple versions of cosmos-sdk and ibc-go, we can only branch out from main, which currently has a version of async-icq that supports ibc-go v7. This means that all three versions of ibc-hooks will live together with async-icq v7, which feels awkward.

We are open to suggestions on how to better manage this, such as having a base branch with all the latest patches for a certain cosmos-sdk/ibc-go release.

We propose having two release branches, release/v6 and release/v7, with each branch containing specific versions of the modules. Pull requests for ibc-hooks module updates targeting ibc-go/v6 will be merged into release/v6, and a tag ibc-hooks/v6.0.1 can be released from release/v6.

- release/v6
    |_ async-icq/v6.0.1
    |_ ibc-hooks/v6.0.0
- release/v7
    |_ async-icq/v7.0.1
    |_ ibc-hooks/v7.0.1

We noticed from testing that the data encoded in the ResponseQuery on the host side was changed to be stored into Key instead of Value when changing versions from v7.0.0 to v7.1.1. The tests were ran using both osmosis and juno, unfortunately cannot provide the e2e test since it is in a private repo currently. We can spin up a test on this repo for further proof if required.

There was no mention of this change in the CHANGELOG and we were wondering whether this change was intentional. Thanks!

TODO

• Get a guide up for adding a new directory (codeowners, coordination, requirements, labels, etc)
• Add Stride write -> @sampocs @asalzmann @riley-stride @shellvish and @ethan-stride
• Diff check feat/v50 branch upstream -> noble-assets fork (this pr)
• Add ibc-rate-limit label, give credit to original authors
• Write in basic e2e interchaintest if not already (take from spawn). Add to GH CI
  • Migrate over original bash scripts to ict
• Remove v50 from upstream / add to readme to use ibc-apps Archive upstream
• Tag a v8.0.0 release for ibc-rate-limit
• Anywhere that Stride-Labs/ibc-rate-limiting is needs to be renamed (i.e. docs)

From: strangelove-ventures/packet-forward-middleware#90

if pfm or icq labels are added, add the event to the motherboard

Affected applications:

• Packet Forward Middleware
• Async-ICQ

Reason:

SDK v46 deprecated this module, and it is being removed in SDK 48/50.

Solution

Migrate from the x/params module to being saved within the keeper

Since this repo only contains sdk modules directly integrated with ibc, meanwhile we have many great modules that do not use ibc directly (like token factory, oracle, alliance) but still used by many chains, maybe it's time to create a repo for those ones? Hopefully one day we have substrate marketplace equivalent.

reference: CosmosContracts/juno#1044

Ensure to keep PFM higher in the stack compared to other middlewares / modules. This way custom implementation such as ICS29 do not break due to custom handler methods PFM does not support (and never will or should)

Found by: Luca @ informal

• put the softwhere where it oughtta be
• update the List of Apps / Ecosystem Apps tables in the README.

async-icq has the concept of a host chain (the chain where queries are received and processed) and a controller chain (the chain sending the query req and processing the ack). there is a demo that currently lives here but i think it would be more beneficial for app developers to have access to this demo in the ibc-apps repo. it would also make testing via an e2e test with ictest in this repo straightforward so that functionality for both the host and controller can be exercised in CI

From Justin in the previous repo: strangelove-ventures/packet-forward-middleware#10

router is used in a few different contexts across modules/in the SDK so perhaps renaming to ibcrouter would be less confusing

"I think pfm or forward are valid choices! typically when I import the middleware into other repos I end up creating an alias called forward"

"I think a lot of apps already rename the import to packetforward so this might be a good candidate as well"

Hello, what is planned fix for https://github.com/SCV-Security/responsible-disclosures/blob/main/SCV%20-%20Responsible%20Disclosure%20-%20PFM%20and%20IBC-hooks.pdf ?
I though of all fixes I can do in contract with all data available, the only fix CW developer can do is signed token inside packet, but without ZK this is dead end for IBCing high gas cost chains with different signature schemes.
Planned fix would help to write code the way anticipating that fix to happen.
Thank you

• put the softwhere where it oughtta be
• update the List of Apps / Ecosystem Apps tables in the README.

I'm currently assisting the Provenance Blockchain in migrating to sdk 50. One step in this migration process is the ability to remove github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1 from go.mod since cosmos/gogoproto is being used. The Provenance Blockchain cannot remove this dependency because it was not removed from async-icq/v8. This ibc-app should be able to remove it since it is no longer needed.

From Jake / noah,

https://github.com/DA0-DA0/polytone

TODO

• Wiki Content copy paste
• issue pull over
• Add to @cosmos/daodao_ibc-apps_write -> Noah, Jake, Artimix, and bekauz
• getting them in as codeowners
• Artifact Publish on tag of cosmwasm/polytone

Right now, the ForwardPacket doesnt allow to pass any other attributes other than the ones it already has which makes passing memo to the destination chain not possible. Adding a memo which is preserved during the hop would be ideal like the next attribute.

I was trying to understand meaning of refundTimeout inside IBCMiddleware but didn't find any usage of this attribute except initialization (3 times) and unit test (2 times) - see screenshot.

I'm still wondering what is the reason behind and if it should be present at all.

Get CI from #179 (chores.yml)

backport to

• v7
• v6
• v5
• v4

We are adding PFM to Stride next week.

In v7.1.2, the integrations docs say

// Register packet forward middleware AppModule
app.moduleManager = module.NewManager(
    ...
    packetforward.NewAppModule(app.PacketForwardKeeper),
)

But a second argument is required by NewAppModule

// NewAppModule creates a new packetforward module
func NewAppModule(k *keeper.Keeper, ss exported.Subspace) AppModule {
	return AppModule{
		keeper:         k,
		legacySubspace: ss,
	}
}

What should legacySubspace be? Why is it required? @Reecepbcups

Related PR: #128

Context

Problem

The first link is a closed PR and the second link is broken.

Proposal

The first link may still relevant be if that was the original PR that was the basis for this line of work but it seems confusing without context. The second link seems like it should be updated or removed.

Related: this repo may consider adopting markdown link check to get CI signal when a link is dead.

We should bring the packet forward middleware over, along with any supported branches. Tags should be created for each supported branch as well

https://github.com/strangelove-ventures/packet-forward-middleware

At the moment, if the param is not set (i.e.: set to "") txs with forwarding will fail since https://github.com/cosmos/ibc-apps/blob/main/middleware/packet-forward-middleware/router/keeper/keeper.go#L214 can't convert it to a Dec.

Adding sensible defaults should be an easy fix

• put the softwhere where it oughtta be
• update the List of Apps / Ecosystem Apps tables in the README.

We should add mergify to the repo to assist in back porting issues across supported branches/releases. It has been a huge help in other repos and I can see it being especially useful here as well

This code should error because we can't instantiate code_id 2000, because it doesn't exist

{
  "contract": "migaloo1v7cuk85rgfrsegsegsslre0lhgrvq28gqtlyms6",
  "msg": {
    "execute_msgs": {
      "msgs": [
        {
          "msg": {
            "wasm": {
              "instantiate": {
                "code_id": 2000,
                "msg": "eyJhbGxvd19jcm9zc19jaGFpbl9tc2dzIjogdHJ1ZX0=",
                "funds": [],
                "label": "Global proxy"
              }
            }
          }
        }
      ]
    }
  }
}

I'm not sure if this issue "lives" here or possibly in CosmWasm. As we work on it, we'll put updates here.

Reference

#41

Goal

The ability to use an ICS20 packet to directly stake w/o additional transactions from the end user. Designed to be chained with other middlewares, such as packet forward middleware and/or swap and forward

Flow

$ATOM on Osmosis -> IBC out to Cosmoshub-4 -> stake to validator with X amount

Why?

Better UX, good base middleware which can be easily copied for other teams to build application specific middlewares (lending, borrowing, swaps, etc.)

There are 2 undone issues to handle relayer incentives it on CW level:

CosmWasm/cosmwasm#1491

CosmWasm/cosmwasm#893

Incentive - why would relayer burn gas on wasm hook packet?

One solution could be packet fee takes from ics20, but that is per chain module and does not work in all cases.

So how I can within existing wasm hook do incentives?

Can my contract reflect (query current TX context) on signer (get signer infos) in Cosmos chain so that my contract can bank transfer some?

Gas prices spiked recently on bull market.

ICS27 has solution has something about fees, but its callback sucks (it is 2x more blocks slowness on top of existing wasm hook).

Hello! Recent changes in ibc-go 7.1.0 (see apps/transfer in release notes and specifically ADR-011 for the background) are breaking unit tests for ibc-hooks package. While it can still work fine with ibc-go 7.2.0, this mock of escrow in the test is not sufficient anymore:

Will result in a panic like this:

receiver cosmos1axvk83fmkhz98wqzse6ds0aq53q99xs7cc0478yungfeflscex0qlg7stl unescrows 1stake
    suite.go:87: test panicked: negative coin amount: -1stake
        goroutine 12 [running]:
        runtime/debug.Stack()
        	/usr/local/go/src/runtime/debug/stack.go:24 +0x64

This is because escrow amounts now tracked in TransferKeeper as well, not only as balances in the bank module.

To properly simulate escrowing tokens without actually sending them, the caller must also set the total escrow amount in the TransferKeeper directly:

// SetTotalEscrowForDenom stores the total amount of source chain tokens that are in escrow.
// Amount is stored in state if and only if it is not equal to zero. The function will panic
// if the amount is negative.
func (k Keeper) SetTotalEscrowForDenom(ctx sdk.Context, coin sdk.Coin) 

I have made necessary changes in PR #56 please note that it supports versions prior to 7.1.0 as well, that don't have SetTotalEscrowForDenom available in TransferKeeper.

There should be CI testing for each application and module.

We should document and standardize this process and make is smooth for future incoming apps.

Some R and D may be necessary for mono repos like this.

ref: #176
https://github.com/tcort/markdown-link-check on every push?

Here https://github.com/cosmos/ibc-apps/blob/main/modules/ibc-hooks/tests/unit/testdata/counter/README.md you mention that

This way we can verify that, independently of the sender, the funds will end up under the WasmHooksModuleAccount address when the contract is executed via an IBC send that goes through the wasmhooks module.

But in reality I can see that module account never receives funds. Instead intermediary account is created from channel+original sender and it temporary holds funds.

Proofs:

// Calculate the receiver / contract caller based on the packet's channel and sender
channel := packet.GetDestChannel()
sender := data.GetSender()
senderBech32, err := keeper.DeriveIntermediateSender(channel, sender, h.bech32PrefixAccAddr)
if err != nil {
return NewEmitErrorAcknowledgement(ctx, types.ErrBadSender, fmt.Sprintf("cannot convert sender address %s/%s to bech32: %s", channel, sender, err.Error()))
}

// The funds sent on this packet need to be transferred to the intermediary account for the sender.
// For this, we override the ICS20 packet's Receiver (essentially hijacking the funds to this new address)
// and execute the underlying OnRecvPacket() call (which should eventually land on the transfer app's
// relay.go and send the sunds to the intermediary account.
//
// If that succeeds, we make the contract call

https://github.com/cosmos/ibc-apps/blob/main/modules/ibc-hooks/wasm_hook.go#L71

IMO the README should be rephrased.

Issue

Because of the number of docker network nodes setup:

• Interchaintest with PFM is flakey (github network limitation)
• PFM test are slow (IBC + single core for multiple networks + relayer)

Solution

Interchaintest added cometmock support earlier in the year here. We should see if using it allows for nearly instant block times.

This way test are drastically faster & if they do fail, it should be 1-2m instead of ~5-6m

Implementation note

NOTE: relayer will need to have a faster min-loop-duration to keep up.

We are in the process of updating Provenance Blockchain to cosmos sdk v0.50.0, and one step involves updating ibc-go to v8. One of our dependencies is async-icq, and unfortunately it doesn't have support for ibc-go/v8 yet. A new tagged version of async-icq/v8 would be very helpful.