Alex ibc spike2 #231

ethanfrey · 2020-07-27T18:23:49Z

This is not my branch, but I made a PR so I can comment on this better and get more visibility

ethanfrey · 2020-07-27T18:45:54Z

Okay, some learnings of mine going through this....

OnAcknowledgementPacket is called every on both success and failure. This makes the callback flow a bit more confusing.

Exactly one callback will come from a given packet. If OnTimeoutPacket is called, then we treat it as an error. If OnAcknowledgementPacket is called, then we check the data packet and handle either success or error.

This does make sense for lower-level coding, but for the contracts, it would be nicer to expose something like onAckSucceed with only success packets and onAckError with only failed packets. I dream of making some standard envelope for OnAcknowledgementPacket such that we can parse success/error in a generic way, then pass arbitrary "sucess bytes" to the success case.

(The flow and use of returning errors or returning a packet with an error code is explained here: https://docs.cosmos.network/master/ibc/custom.html#receiving-packets)

ethanfrey

Interesting spike.

Added quite some comments there. I think it would be best to take all these ideas and formalize them in #214 before getting too much deeper in the implementation, but doing this spike did brings up a lot of good questions on the specifics.

ethanfrey · 2020-07-27T18:59:41Z

x/wasm/internal/keeper/cosmwasm/contract.go

+	sdk "github.com/cosmos/cosmos-sdk/types"
+)
+
+type OnReceiveIBCResponse struct {


These are three clear entrypoints. I would like to make OnAcknowledgeIBCResponse / OnTimeoutIBCResponse => OnAcknowledgeIBCSuccess / OnAcknowledgeIBCError as I see no difference in handling a timeout from a rejected packet (it failed, undo state changes)

🤔 Success and Error are a bit ambiguous as an Ack can contain an application level error message.

Exactly, the IBC/wasmd entry points make sense for the protocol level.

I would like to use more semantically interesting ones for the contracts.
And yes, I am only thinking of the interface we expose to the contracts, and figure we provide an adaptor between the official IBC and our desired API.

ethanfrey · 2020-07-27T19:00:09Z

x/wasm/internal/keeper/cosmwasm/contract.go

+	Log []wasmTypes.LogAttribute `json:"log"`
+}
+
+type AcceptChannelResponse struct {


This would be one more contract callback? for the OnTryInitChannel or whatever it is called?

I have used this for OnChanOpenInit OnChanOpenTry but channel handshake was only touched in this spike.
My idea was to have the contracts engaged here as well. For example if a contract is end-of-life (burned, escrow released,...) it would make sense to not accept any new channels.

ethanfrey · 2020-07-27T19:00:43Z

x/wasm/internal/keeper/cosmwasm/query.go

@@ -0,0 +1,18 @@
+package cosmwasm
+
+type IBCQuery struct {


Yeah, didn't even think about the queries.

This is a good point, we should define them well.

Not sure if this is a useful scenario but I thought a contract may want to query all it's channels and broadcast to it's counter parties. Or may close them all (end-of-life scenario)

ethanfrey · 2020-07-27T19:04:26Z

x/wasm/internal/keeper/ibc.go

+	// IBC channel livecycle
+	AcceptChannel(hash []byte, params cosmwasm.Env, order channeltypes.Order, version string, connectionHops []string, store prefix.Store, api wasm.GoAPI, querier QueryHandler, meter sdk.GasMeter, gas uint64) (*cosmwasm.AcceptChannelResponse, uint64, error)
+	//OnConnect(hash []byte, params cosmwasm.Env, msg []byte, store prefix.Store, api wasm.GoAPI, querier QueryHandler, meter sdk.GasMeter, gas uint64) (*cosmwasm.OnTimeoutIBCResponse, uint64, error)
+	//OnClose(hash []byte, params cosmwasm.Env, msg []byte, store prefix.Store, api wasm.GoAPI, querier QueryHandler, meter sdk.GasMeter, gas uint64) (*cosmwasm.OnTimeoutIBCResponse, uint64, error)


Hmmm.... we probably do want this one.

I'm not sure when OnConnect would be called, or what it would be used for. You can get the same info from an IBCQuery and I don't want to give the contracts too much agency to randomly run code in arbitrary lifecycle events.

But if you have a communication channel with another module and it closes, it is probably nice to allow some cleanup? Or will I simply see the channel gone on the next query and receive OnTimeout events for all pending packets? That may be enough for cleanup, but I could see wanting to do some cleanup.

The issue would be the contract just erroring on OnClose and never letting the IBC module do the cleanup. Hmmm...

You can get the same info from an IBCQuery

With a query I would get the channel date but I can only act on some other message that would be received later. This callback on the other hand does not need an additional message. I would rather see this as an extension point to let contracts implement their own behaviour and state changes.

ethanfrey · 2020-07-27T19:05:02Z

x/wasm/internal/keeper/ibc.go

+
+var MockContracts = make(map[string]IBCCallbacks, 0)
+
+func (k Keeper) AcceptChannel(ctx sdk.Context, contractAddr sdk.AccAddress, order channeltypes.Order, version string, connectionHops []string, ibcInfo cosmwasm.IBCInfo) ([]string, error) {


we want to enforce order == ORDERED

ethanfrey · 2020-07-27T19:07:45Z

x/wasm/internal/keeper/ibc.go

+	if !ok { // hack for testing without wasmer
+		panic("not supported")
+	}
+	res, gasUsed, execErr := mock.OnReceive(codeInfo.CodeHash, params, payloadData, prefixStore, cosmwasmAPI, querier, ctx.GasMeter(), gas)


This will need to know what channel the message is coming in on. that is not included in payload data. Any other context you think as useful?

I have added this to the environment for the contract.
There are also a couple of message related information that are passed there, too.

Make sense.

I wonder a bit when 90% of the time the Environment doesn't have this info.
Or do you propose two Environment types?

The original cosmwasm.Env that is sent for init/handle/migrate and then what you coded here (maybe rename IBCEnvironment), which is a superset of cosmwasm.Env and passed into the ibc_handle method for example.

This makes it clear what is called when

Ja, why not have different ones as we call different callbacks anyway. Good idea

The IBC PortID should be somehow available to the contract in all environments as well as IBC queries. An outgoing
MsgWasmIBCCall would start in a non IBC env. (if it works as I assume)

ethanfrey · 2020-07-27T19:08:42Z

x/wasm/internal/keeper/ibc.go

+	if !ok {
+		panic("not supported")
+	}
+	res, gasUsed, execErr := mock.OnAcknowledgement(codeInfo.CodeHash, params, payloadData, acknowledgement, prefixStore, cosmwasmAPI, querier, ctx.GasMeter(), gas)


As above, we need to pass minimally the channelID here.

Also, I would love to be able to detect ack as success/error in our go code (in a fixed manner) before sending to the contract

We also need some way to map back to the original packet that we sent. This is not just an unsolicited acknowledgement (like receive), but an acknowledgement that ties to one specific packet.

In ibc transfer they take the packet metadata (port and channel) and use it to determine the refund

The original payload from the contract is passed in the payloadData. The contract has it's source data and the ack so that it can match it proper.
(Assuming the data contains an unique identifier. This is also an area where the rust cosmwasm framework may provide some tooling for non natural identifiers)

The sender's channel/port are in IBCPacketInfo and passed via Environment

Ah... so payload data is read from the local store from the original input, and isn't something sent from the other side (that is acknowledgement)

I have not looked deep enough through the IBC impl to find where the origin message comes from in the ack process but it is available to us in the IBC callbacks.

ethanfrey · 2020-07-27T19:10:04Z

x/wasm/internal/keeper/ibc.go

-	timeoutTimestamp uint64,
-	msg json.RawMessage,
-) error {
+func (k Keeper) IBCCallFromContract(ctx sdk.Context, sourcePort, sourceChannel string, sender sdk.AccAddress, timeoutHeight, timeoutTimestamp uint64, msg json.RawMessage) error {


Cool idea, so this is another CosmosMsg type that the contract can return.

IBCSendPacket and IBCOpenChannel ?

Good to look at this code, I need to combine this with the comments on #214 and get a clear API spec'd out.

I assume that Send and CloseChannel are possible from a contract. The OpenChannel process is not that clear to me!

A channel is based on a connection and that on a client. Both are out of scope of our module so far. In GoS the relayer would establish them based on configuration.
Technically the contract could store and provide the necessary data but calling out to other chains would mix with the relayers job and feels wrong at this stage.

from the cli:
wasmcli tx ibc channel open-init [port-id] [channel-id] [counterparty-port-id] [counterparty-channel-id] [connection-hops] [flags]

From the spec:
The connectionHops stores the list of connection identifiers, in order, along which packets sent on this channel will travel. At the moment this list must be of length 1. In the future multi-hop channels may be supported.

ethanfrey · 2020-07-27T19:11:02Z

x/wasm/internal/keeper/ibc_mocks_test.go

+}
+
+func (c *mockContract) AcceptChannel(hash []byte, params cosmwasm2.Env, order channeltypes.Order, version string, connectionHops []string, store prefix.Store, api cosmwasm.GoAPI, querier keeper.QueryHandler, meter sdk.GasMeter, gas uint64) (*cosmwasm2.AcceptChannelResponse, uint64, error) {
+	if order != channeltypes.ORDERED {


Hmm... maybe we let the contract use UNORDERED as well?

x/wasm/internal/keeper/ibc_mocks_test.go

alpe · 2020-07-28T12:42:56Z

OnAcknowledgementPacket is called every on both success and failure. This makes the callback flow a bit more confusing.

This is the application level. An error like "ErrInsufficientFunds" in ibc-transfer would be handled there. I think about this as an RPC call where the other side returns some method specific values. The response can be like (ok, error) or more complex. It would also serve as a commit in a DB transaction. Regardless of the payload, the IBC package was delivered successfully.

The OnTimeoutPacket is the transaction rollback equivalent.

I assume there can be IBC related errors like "the port is no longer able to accept channels, such as if the Connection to the remote chain has failed, perhaps because a consensus failure was observed" as the third group.

I dream of making some standard envelope for OnAcknowledgementPacket such that we can parse success/error in a generic way, then pass arbitrary "sucess bytes" to the success case.

I see this in the cosmwasm rust framework layer. We could provide handlers and other tooling there and keep wasmd as dynamic as possible

ethanfrey · 2020-07-28T13:36:06Z

I see this in the cosmwasm rust framework layer. We could provide handlers and other tooling there and keep wasmd as dynamic as possible

wasmd is likely the only flex point the blockchain developer has and the contract developer can only build upon what cosmwasm-std provides.

You suggest the mapping of ack -> success/error should happen inside the contract itself, not in the wasm framework?

alpe · 2020-07-28T13:47:45Z

You suggest the mapping of ack -> success/error should happen inside the contract itself, not in the wasm framework?

Ja, this is application layer logic in my understanding. But we can help the contract developer by providing a rust framework/toolset that comes with some defaults/ conventions to make the real contract code simple and fast to build.
If the rust contract framework provides a default ack frame then we could read from that and pass to simpler methods.
The challenge is to come up with good and reusable components. This will evolve with the use cases IMHO.

ethanfrey · 2020-08-18T09:27:23Z

Closes in favor of #253

alpe added 2 commits July 24, 2020 17:07

IBC to mock contract

00fc3e2

Other ideas

7e8e099

ethanfrey commented Jul 27, 2020

View reviewed changes

alpe mentioned this pull request Aug 18, 2020

Alex ibc spike2d #253

Closed

ethanfrey closed this Aug 18, 2020

ethanfrey deleted the alex_ibc_spike2 branch August 18, 2020 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alex ibc spike2 #231

Alex ibc spike2 #231

ethanfrey commented Jul 27, 2020

ethanfrey commented Jul 27, 2020 •

edited

Loading

ethanfrey left a comment

ethanfrey Jul 27, 2020

alpe Jul 28, 2020

ethanfrey Jul 28, 2020

ethanfrey Jul 27, 2020

alpe Jul 28, 2020

ethanfrey Jul 27, 2020

alpe Jul 28, 2020

ethanfrey Jul 27, 2020

alpe Jul 28, 2020 •

edited

Loading

ethanfrey Jul 27, 2020

ethanfrey Jul 27, 2020

alpe Jul 28, 2020

ethanfrey Jul 28, 2020

alpe Jul 28, 2020

ethanfrey Jul 27, 2020

ethanfrey Jul 27, 2020

alpe Jul 28, 2020

alpe Jul 28, 2020

ethanfrey Jul 28, 2020

alpe Jul 28, 2020

ethanfrey Jul 27, 2020

alpe Jul 28, 2020

ethanfrey Jul 27, 2020

alpe commented Jul 28, 2020

ethanfrey commented Jul 28, 2020

alpe commented Jul 28, 2020

ethanfrey commented Aug 18, 2020


		var MockContracts = make(map[string]IBCCallbacks, 0)

		func (k Keeper) AcceptChannel(ctx sdk.Context, contractAddr sdk.AccAddress, order channeltypes.Order, version string, connectionHops []string, ibcInfo cosmwasm.IBCInfo) ([]string, error) {

Alex ibc spike2 #231

Alex ibc spike2 #231

Conversation

ethanfrey commented Jul 27, 2020

ethanfrey commented Jul 27, 2020 • edited Loading

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpe Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpe commented Jul 28, 2020

ethanfrey commented Jul 28, 2020

alpe commented Jul 28, 2020

ethanfrey commented Aug 18, 2020

ethanfrey commented Jul 27, 2020 •

edited

Loading

alpe Jul 28, 2020 •

edited

Loading