search

CosmWasm / wasmd

Basic cosmos-sdk app with web assembly smart contracts
Other
366 stars 397 forks source link

Async Ack #1876

Closed chipshort closed 3 months ago

chipshort commented 5 months ago

also fixes #1891

I had to do changes in quite a few places

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 38.31776% with 66 lines in your changes missing coverage. Please review.

Project coverage is 54.68%. Comparing base (5ae2f4a) to head (eeb3cd4).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1876/graphs/tree.svg?width=650&height=150&src=pr&token=rxXgFH3QTf&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm)](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1876?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm) ```diff @@ Coverage Diff @@ ## main #1876 +/- ## ========================================== - Coverage 54.87% 54.68% -0.20% ========================================== Files 65 65 Lines 9775 9857 +82 ========================================== + Hits 5364 5390 +26 - Misses 3866 3919 +53 - Partials 545 548 +3 ``` | [Files](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1876?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm) | Coverage Δ | | |---|---|---| | [x/wasm/client/cli/tx.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1876?src=pr&el=tree&filepath=x%2Fwasm%2Fclient%2Fcli%2Ftx.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-eC93YXNtL2NsaWVudC9jbGkvdHguZ28=) | `15.18% <ø> (ø)` | | | [x/wasm/keeper/keeper\_cgo.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1876?src=pr&el=tree&filepath=x%2Fwasm%2Fkeeper%2Fkeeper_cgo.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-eC93YXNtL2tlZXBlci9rZWVwZXJfY2dvLmdv) | `94.11% <100.00%> (ø)` | | | [x/wasm/keeper/relay.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1876?src=pr&el=tree&filepath=x%2Fwasm%2Fkeeper%2Frelay.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-eC93YXNtL2tlZXBlci9yZWxheS5nbw==) | `79.38% <68.42%> (-1.87%)` | :arrow_down: | | [x/wasm/types/keys.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1876?src=pr&el=tree&filepath=x%2Fwasm%2Ftypes%2Fkeys.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-eC93YXNtL3R5cGVzL2tleXMuZ28=) | `52.30% <0.00%> (-8.41%)` | :arrow_down: | | [x/wasm/keeper/keeper.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1876?src=pr&el=tree&filepath=x%2Fwasm%2Fkeeper%2Fkeeper.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-eC93YXNtL2tlZXBlci9rZWVwZXIuZ28=) | `76.82% <41.66%> (-1.14%)` | :arrow_down: | | [x/wasm/keeper/handler\_plugin.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1876?src=pr&el=tree&filepath=x%2Fwasm%2Fkeeper%2Fhandler_plugin.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-eC93YXNtL2tlZXBlci9oYW5kbGVyX3BsdWdpbi5nbw==) | `64.70% <31.48%> (-15.49%)` | :arrow_down: |
chipshort commented 3 months ago

how to cleanup this temporary packet store at some point for the packets that get never acknowledged. Would be great to have some sort of configurable lifetime.

That's a general problem with async acks, even on the ibc-go layer. If there is no ack, the packet commitment is not deleted there either. Adding a lifetime opens a whole different can of worms. When would we clean it up? Also, I'm not sure how a valid contract should handle that case. After deleting the stored packet, there is no way to ack the packet anymore, so that can in theory cause a packet that would otherwise have been acknowledged to get stuck.

I'm not convinced there's a good solution for this, so my take is to just live with the fact that incorrect contracts will write packets that are never cleaned up (just like they can write stuff into their own state that never gets cleaned up).

webmaster128 commented 3 months ago

my take is to just live with the fact that incorrect contracts will write packets that are never cleaned up

Right, sounds good. In that case should we add a storage write fee to StoreAsyncAckPacket? Or does prefixStore.Set(key, packetBz) charge gas automatically?

chipshort commented 3 months ago

Yes prefixStore.Set(key, packetBz) will charge that. You can see an example in the test in relay_test.go.