ICS-030 Middleware support #184

bluele · 2023-04-26T11:13:18Z

https://github.com/cosmos/ibc/tree/main/spec/app/ics-030-middleware

bluele · 2023-10-20T09:15:35Z

I wrote an example of a design pattern for middleware contracts in the following gist: https://gist.github.com/bluele/39378422eb8c1cd55dfa5702b43978e0

In this example, I demonstrate the implementation of middleware that can be applied to an existing App contract by utilizing solidity inheritance.

I believe this approach offers several advantages compared to using multiple contracts(e.g. #215):

avoid unnecessary contract calls and valid caller checks
easy to implement upgradability as it is a single contract

dzmitry-lahoda · 2023-10-20T10:03:45Z

okey, so super will call stack according order inheritance was done.

dzmitry-lahoda · 2023-10-20T10:05:44Z

i have read the code. it allows to reuse middleware for different apps and allows ro.stack middlewares to be executed in order via super. so it looms good.

dzmitry-lahoda · 2023-10-20T10:07:41Z

will redo like this approach with test.

dzmitry-lahoda · 2023-10-20T10:33:51Z

@bluele

    function sendPacket(Lib.Packet memory packet)
        internal
        virtual
        override(AppBase, HookMiddlewarePacketSender, FeeMiddlewarePacketSender)
    {
        // fee -> hook -> app -> base
        super.sendPacket(packet);
    }

looks wrong.

app should call sendPacket of parent, so it is no super call in parent. but mole like child in parent.

dzmitry-lahoda · 2023-10-20T10:34:35Z

on receive fee -> hook -> app -> base correct,

or send base -> app -> hook -> fee is correct

dzmitry-lahoda · 2023-10-20T10:36:51Z

When nesting an application, the module must make sure that it is in the middle of communication between core IBC and the application in both directions. Developers should do this by registering the top-level module directly with the IBC router (not any nested applications). The nested applications in turn, must be given access only to the middleware's WriteAcknowledgement and SendPacket rather than to the core IBC handlers directly.

will it be ok to do hybrid?

super, in one direction, but still need to pass child address to its parent address to call.

will modify like that.

bluele · 2023-10-20T11:38:58Z

@dzmitry-lahoda

// fee -> hook -> app -> base
looks wrong

Ah, sorry, that code comment is confusing.
I have fixed https://gist.github.com/bluele/39378422eb8c1cd55dfa5702b43978e0

on receive fee -> hook -> app -> base correct,
or send base -> app -> hook -> fee is correct

Exactly, middleware's sendPacket calling order must be in reverse order of the receivePacket calling order.

Therefore, the inheritance order of the Sender and Receiver of each middleware must be reversed:
https://gist.github.com/bluele/39378422eb8c1cd55dfa5702b43978e0#file-ibc_middleware_example-sol-L134-L137

The execution result may be easier to understand:
// send: app.sendTransfer -> fee.send -> hook.send -> base.send(ibc handler)
// recv: hook.recv -> fee.recv -> app.recv

forge test -vvvv --match-test test_app
[⠰] Compiling...
No files changed, compilation skipped

Running 1 test for test/Middleware.t.sol:MiddlewareTest
[PASS] test_app() (gas: 645363)
Logs:
  App: sendTransfer called
  FeeMiddlewarePacketSender: send packet/hello
  HookMiddlewarePacketSender: send packet/hello_hooked
  Base: send packet via IBC Handler/hello_hooked
  =================
  HookMiddlewarePacketReceiver: recv packet/hello_hooked
  FeeMiddlewarePacketReceiver: recv packet/hello_hooked
  App: recv packet/hello_hooked
  FeeMiddlewarePacketReceiver: after recv packet
  HookMiddlewarePacketReceiver: after recv packet

Traces:
  [645363] MiddlewareTest::test_app()
    ├─ [592610] → new HookFeeMiddlewaredApp@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
    │   └─ ← 2844 bytes of code
    ├─ [8422] HookFeeMiddlewaredApp::sendTransfer(hello)
    │   ├─ [0] console::log(App: sendTransfer called) [staticcall]
    │   │   └─ ← ()
    │   ├─ [0] console::log(FeeMiddlewarePacketSender: send packet/hello) [staticcall]
    │   │   └─ ← ()
    │   ├─ [0] console::log(HookMiddlewarePacketSender: send packet/hello_hooked) [staticcall]
    │   │   └─ ← ()
    │   ├─ [0] console::log(Base: send packet via IBC Handler/hello_hooked) [staticcall]
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [0] console::log(=================) [staticcall]
    │   └─ ← ()
    ├─ [9338] HookFeeMiddlewaredApp::onRecvPacket((0x68656c6c6f5f686f6f6b6564))
    │   ├─ [0] console::log(HookMiddlewarePacketReceiver: recv packet/hello_hooked) [staticcall]
    │   │   └─ ← ()
    │   ├─ [0] console::log(FeeMiddlewarePacketReceiver: recv packet/hello_hooked) [staticcall]
    │   │   └─ ← ()
    │   ├─ [0] console::log(App: recv packet/hello_hooked) [staticcall]
    │   │   └─ ← ()
    │   ├─ [0] console::log(FeeMiddlewarePacketReceiver: after recv packet) [staticcall]
    │   │   └─ ← ()
    │   ├─ [0] console::log(HookMiddlewarePacketReceiver: after recv packet) [staticcall]
    │   │   └─ ← ()
    │   └─ ← 1
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.13ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

bluele · 2023-10-20T12:01:08Z

To make this design pattern available, the following modifications would be required:

implement sendPacket to the IBCAppBase
modify Apps(i.e. ics-20) to call sendPacket when sending packets

dzmitry-lahoda · 2023-10-20T13:01:23Z

WOW. I did know about override feature. trying.

dzmitry-lahoda · 2023-10-20T17:08:39Z

2 issue with approach:

need to make all packet data to be memory
that leads to next via-ir issue
also need to override ALL methods in middleware (as need to specify selector)

but it gives single contract deployment of app stack - i guess it is way better.

dzmitry-lahoda · 2023-10-20T17:59:38Z

@bluele so in this #220

needed to split app base and app base with defaults, so middlewares do no need to specify ALL dummy methods
via-ir issue.

if any hint solve via-ir?

dzmitry-lahoda · 2023-10-20T18:04:12Z

    coordinator.go:501: ack found: {"result":"AQ=="}
    coordinator.go:510: delay for ack@3018 40.852138202s
=== RUN   TestChainTestSuite/TestTimeoutAndClose
    suite.go:87: test panicked: Account not found
        goroutine 98 [running]:
        runtime/debug.Stack()
                /nix/store/23wrmhyfnwjxc9cvy3lab9xj551ic53c-go-1.20.8/share/go/src/runtime/debug/stack.go:24 +0x65
        github.com/stretchr/testify/suite.failOnPanic(0xc00091c820, {0x1251420, 0xc00071a9c0})
                /home/dz/go/pkg/mod/github.com/stretchr/[email protected]/suite/suite.go:87 +0x3b
        github.com/stretchr/testify/suite.Run.func1.1()
                /home/dz/go/pkg/mod/github.com/stretchr/[email protected]/suite/suite.go:183 +0x252
        panic({0x1251420, 0xc00071a9c0})
                /nix/store/23wrmhyfnwjxc9cvy3lab9xj551ic53c-go-1.20.8/share/go/src/runtime/panic.go:884 +0x213
        github.com/hyperledger-labs/yui-ibc-solidity/pkg/testing.(*Chain).UpdateLCInputData(0xc00091c9c0)
                /home/dz/github.com/hyperledger-labs/yui-ibc-solidity/pkg/testing/chains.go:390 +0x1e5
        github.com/hyperledger-labs/yui-ibc-solidity/pkg/testing.NewCoordinator(...)
                /home/dz/github.com/hyperledger-labs/yui-ibc-solidity/pkg/testing/coordinator.go:23
        github.com/hyperledger-labs/yui-ibc-solidity/tests/e2e.(*ChainTestSuite).TestTimeoutAndClose(0xc0006e4f80)
                /home/dz/github.com/hyperledger-labs/yui-ibc-solidity/tests/e2e/chains_test.go:192 +0x285
        reflect.Value.call({0xc0000bcb80?, 0xc0001334e8?, 0x13?}, {0x13c5e37, 0x4}, {0xc0000a0e70, 0x1, 0x1?})
                /nix/store/23wrmhyfnwjxc9cvy3lab9xj551ic53c-go-1.20.8/share/go/src/reflect/value.go:586 +0xb07
        reflect.Value.Call({0xc0000bcb80?, 0xc0001334e8?, 0xc0006e4f80?}, {0xc0004e6e70?, 0x1db4600?, 0x17e9a2f?})
                /nix/store/23wrmhyfnwjxc9cvy3lab9xj551ic53c-go-1.20.8/share/go/src/reflect/value.go:370 +0xbc
        github.com/stretchr/testify/suite.Run.func1(0xc00091c820)
                /home/dz/go/pkg/mod/github.com/stretchr/[email protected]/suite/suite.go:197 +0x4b6
        testing.tRunner(0xc00091c820, 0xc0009ea000)
                /nix/store/23wrmhyfnwjxc9cvy3lab9xj551ic53c-go-1.20.8/share/go/src/testing/testing.go:1576 +0x10b
        created by testing.(*T).Run
                /nix/store/23wrmhyfnwjxc9cvy3lab9xj551ic53c-go-1.20.8/share/go/src/testing/testing.go:1629 +0x3ea
--- FAIL: TestChainTestSuite (138.35s)
    --- PASS: TestChainTestSuite/TestICS20 (39.67s)
    --- PASS: TestChainTestSuite/TestPacketRelayWithDelay (98.66s)
    --- FAIL: TestChainTestSuite/TestTimeoutAndClose (0.02s)
FAIL
FAIL    github.com/hyperledger-labs/yui-ibc-solidity/tests/e2e  138.392s
FAIL
make: *** [Makefile:85: e2e-test] Error 1

ICS20 works as before. i doubt timeout relats to my changs.

ICS20 wihtout middleware

dzmitry-lahoda · 2023-10-20T18:04:46Z

For more information, try '--help'.
bash-5.2$ forge build
[⠰] Compiling...
[⠑] Compiling 85 files with 0.8.21
[⠢] Solc 0.8.21 finished in 5.29s
Error: 
Compiler run failed:
Error: Compiler error (/solidity/libsolidity/codegen/LValue.cpp:52):Stack too deep. Try compiling with `--via-ir` (cli) or the equivalent `viaIR: true` (standard JSON) while enabling the optimizer. Otherwise, try removing local variables.
   --> contracts/apps/hooks/osmosis.sol:133:74:
    |
133 |         return (Osmosis.marshalResult(Osmosis.marshalContractAck(result, acknowledgement)), true);
    |                                                                          ^^^^^^^^^^^^^^^
For more information, try '--help'.
bash-5.2$ forge build
[⠰] Compiling...
[⠑] Compiling 85 files with 0.8.21
[⠢] Solc 0.8.21 finished in 5.29s
Error: 
Compiler run failed:
Error: Compiler error (/solidity/libsolidity/codegen/LValue.cpp:52):Stack too deep. Try compiling with `--via-ir` (cli) or the equivalent `viaIR: true` (standard JSON) while enabling the optimizer. Otherwise, try removing local variables.
   --> contracts/apps/hooks/osmosis.sol:133:74:
    |
133 |         return (Osmosis.marshalResult(Osmosis.marshalContractAck(result, acknowledgement)), true);
    |                                                                          ^^^^^^^^^^^^^^^

``

dzmitry-lahoda · 2023-10-20T18:09:36Z

fixed via ir.

unit tests passed.

i will test middleware on REAL input from Centauri and Osmosis cosmos chain soon. will add that copy into unit test after

bluele added the IBC Compatibility with IBC label Apr 26, 2023

dzmitry-lahoda mentioned this issue Oct 4, 2023

feat: ics30 interfaces #215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICS-030 Middleware support #184

ICS-030 Middleware support #184

bluele commented Apr 26, 2023

bluele commented Oct 20, 2023 •

edited

Loading

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

bluele commented Oct 20, 2023

bluele commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

ICS-030 Middleware support #184

ICS-030 Middleware support #184

Comments

bluele commented Apr 26, 2023

bluele commented Oct 20, 2023 • edited Loading

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

bluele commented Oct 20, 2023

bluele commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

dzmitry-lahoda commented Oct 20, 2023

bluele commented Oct 20, 2023 •

edited

Loading