Propose contract methods #258

alpe · 2020-08-20T13:32:58Z

Design goals:

close to other cosmwasm signatures like a return type per method
Use of Environment for IBC related metadata

codecov · 2020-08-20T13:33:09Z

Codecov Report

Merging #258 into 0.10_to_cosmos-stargate_ce9c2b2 will decrease coverage by 0.01%.
The diff coverage is n/a.

@@                         Coverage Diff                         @@
##           0.10_to_cosmos-stargate_ce9c2b2     #258      +/-   ##
===================================================================
- Coverage                            17.68%   17.66%   -0.02%     
===================================================================
  Files                                   32       32              
  Lines                                10593    10593              
===================================================================
- Hits                                  1873     1871       -2     
- Misses                                8636     8637       +1     
- Partials                                84       85       +1

Impacted Files	Coverage Δ
x/wasm/internal/keeper/keeper.go	`91.17% <0.00%> (-0.54%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81b8138...f5f7758. Read the comment docs.

ethanfrey

Made a number of comments on the API details.

We cannot pass tons of arguements to wasm contracts. We need to wrap things into structs we json-encode and send down the line. Made a number of suggestions to improve the API

ethanfrey · 2020-08-20T14:27:42Z

x/wasm/IBC.md

+}
+
+// Environment data for a contract
+type Env struct {


Okay, this is supposed to extend the normla cosmwasm Env with this extra info?

ethanfrey · 2020-08-20T14:29:11Z

x/wasm/IBC.md

+  // Channel to the contract
+  Channel string         `json:"channel"`
+  // Optional packet meta data when called on IBC packet functions
+  Packet  *IBCPacketInfo `json:"packet,omitempty"`


3 levels Env -> IBCInfo -> IBCPacketInfo seems overly complex. I had a much simpler version that covers most of this.

Let's flatten it, and we don't need both (Port, Channel) and (SourcePort, SourceChannel). Just show the local side and they can use GetCounterParty if they need it. Let's trim this to the essential

Okay, this is passed on all calls, connections as well as packets, which is why you have the PacketInfo optional.

This is getting confusing. I would like things always there or not - clear at compile time - not based on out-of-bands understanding

Maybe just wrap the Packet Info around the Msg

ethanfrey · 2020-08-20T14:29:49Z

x/wasm/IBC.md

-    ChannelID string
+type IBCChannelOpenResponse struct {
+  // Result contains a boolean if the channel would be accepted
+  Result                       bool   `json:"result"`


I would use Accepted or Approved here. Result implies something more than a boolean, with real data

Maybe:

struct { Success bool Error string }

Where Error is only set if Success is false

ethanfrey · 2020-08-20T14:33:17Z

x/wasm/IBC.md

+  // channel livecycle
+
+  // OnIBCChannelOpen does the protocol version negotiation during channel handshake phase
+  OnIBCChannelOpen(hash []byte, params cosmwasm.Env, order channeltypes.Order, version string, store prefix.Store, api wasm.GoAPI, querier QueryHandler, meter sdk.GasMeter, gas uint64) (*cosmwasm.IBCChannelOpenResponse, uint64, error)


If we stay closer to the go-cosmwasm side, then let's wrap order channeltypes.Order, version string into a struct (all info will pass in som json object, not lots of different args, doesn't work well with wasm)

ethanfrey · 2020-08-20T14:34:10Z

x/wasm/IBC.md

+  // OnIBCChannelOpen does the protocol version negotiation during channel handshake phase
+  OnIBCChannelOpen(hash []byte, params cosmwasm.Env, order channeltypes.Order, version string, store prefix.Store, api wasm.GoAPI, querier QueryHandler, meter sdk.GasMeter, gas uint64) (*cosmwasm.IBCChannelOpenResponse, uint64, error)
+  // OnIBCChannelConnect callback when a IBC channel is established
+  OnIBCChannelConnect(hash []byte, params cosmwasm.Env, counterpartyPortID, counterpartyChannelID string, store prefix.Store, api wasm.GoAPI, querier QueryHandler, meter sdk.GasMeter, gas uint64) (*cosmwasm.IBCChannelConnectResponse, uint64, error)


Same as above (wrap in stuct): counterpartyPortID, counterpartyChannelID string and please add the final version string here.. the second side may change it, so the original side must get that info at the end. And the ordering enum as well - so we have all info in one place when confirming the connection

ethanfrey · 2020-08-20T14:36:17Z

x/wasm/IBC.md

+  // OnIBCChannelConnect callback when a IBC channel is established
+  OnIBCChannelConnect(hash []byte, params cosmwasm.Env, counterpartyPortID, counterpartyChannelID string, store prefix.Store, api wasm.GoAPI, querier QueryHandler, meter sdk.GasMeter, gas uint64) (*cosmwasm.IBCChannelConnectResponse, uint64, error)
+  // OnIBCChannelConnect callback when a IBC channel is closed
+  OnIBCChannelClose(ctx sdk.Context, hash []byte, params cosmwasm.Env, counterpartyPortID, counterpartyChannelID string, store prefix.Store, api wasm.GoAPI, querier QueryHandler, meter sdk.GasMeter, gas uint64) (*cosmwasm.IBCChannelCloseResponse, uint64, error)


counterpartyPortID, counterpartyChannelID string do we need these? We get our own side in Env and can query counterParty if needed. We should just use the channelID locally to look up everything.

I can see passing this explicitly in OnIBCChannelConnect to provide all relevant info on connect to be use for setup if needed. But once it is set up, do we usually care? And if so we can query

ethanfrey · 2020-08-20T14:40:08Z

x/wasm/IBC.md

+  // Package livecycle
+
+  // OnIBCPacketReceive handles an incoming IBC package
+  OnIBCPacketReceive(hash []byte, params cosmwasm.Env, msg []byte, store prefix.Store, api wasm.GoAPI, querier QueryHandler, meter sdk.GasMeter, gas uint64) (*cosmwasm.IBCPacketReceiveResponse, uint64, error)


See comments at the end, please use this instead of msg

type IBCPacket struct { Msg []byte Sequence uint64 TimeoutHeight uint64 // block timestamp (in nanoseconds) after which the packet times out TimeoutTimestamp uint64 }

ethanfrey · 2020-08-20T14:40:40Z

x/wasm/IBC.md

+  // OnIBCPacketAcknowledgement handles a IBC package execution on the counterparty chain
+  OnIBCPacketAcknowledgement(hash []byte, params cosmwasm.Env, originalData []byte, acknowledgement []byte, store prefix.Store, api wasm.GoAPI, querier QueryHandler, meter sdk.GasMeter, gas uint64) (*cosmwasm.IBCPacketAcknowledgementResponse, uint64, error)
+  // OnIBCPacketTimeout reverts state when the IBC package execution does not come in time
+  OnIBCPacketTimeout(hash []byte, params cosmwasm.Env, msg []byte, store prefix.Store, api wasm.GoAPI, querier QueryHandler, meter sdk.GasMeter, gas uint64) (*cosmwasm.IBCPacketTimeoutResponse, uint64, error)


Maybe same as IBCPacket above, or maybe more info?

ethanfrey · 2020-08-20T14:41:18Z

x/wasm/IBC.md

+  // OnIBCPacketReceive handles an incoming IBC package
+  OnIBCPacketReceive(hash []byte, params cosmwasm.Env, msg []byte, store prefix.Store, api wasm.GoAPI, querier QueryHandler, meter sdk.GasMeter, gas uint64) (*cosmwasm.IBCPacketReceiveResponse, uint64, error)
+  // OnIBCPacketAcknowledgement handles a IBC package execution on the counterparty chain
+  OnIBCPacketAcknowledgement(hash []byte, params cosmwasm.Env, originalData []byte, acknowledgement []byte, store prefix.Store, api wasm.GoAPI, querier QueryHandler, meter sdk.GasMeter, gas uint64) (*cosmwasm.IBCPacketAcknowledgementResponse, uint64, error)


See comments at the end, please use this for the originalData and acknowledgement

type IBCPacket struct { OriginalData []byte Acknowledgement []byte Sequence uint64 TimeoutHeight uint64 // block timestamp (in nanoseconds) after which the packet times out TimeoutTimestamp uint64 }

alpe · 2020-08-21T11:44:10Z

This was some good feedback! 💐 I will close this for now and do a new PR with some learnings

ethanfrey · 2020-08-21T13:47:27Z

As long as these comments are saved and make it in somehow, happy for a new pr

Bumps [github.com/spf13/viper](https://github.com/spf13/viper) from 1.6.1 to 1.6.2. - [Release notes](https://github.com/spf13/viper/releases) - [Commits](spf13/viper@v1.6.1...v1.6.2) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

…ster * remotes/gaia/master: (38 commits) Merge PR CosmWasm#259: Bump SDK master commit Exec go mod tidy (CosmWasm#267) Merge PR CosmWasm#265: add version v2.0.5 to CHANGELOG Fix broken link in delegator guide (CosmWasm#262) docs: removed redundant readme.md (CosmWasm#261) Merge PR CosmWasm#258: Bump github.com/spf13/viper from 1.6.1 to 1.6.2 Merge PR CosmWasm#250: Docs-update Merge PR CosmWasm#256: Bump github.com/pkg/errors from 0.9.0 to 0.9.1 Merge PR CosmWasm#255: Bump github.com/pkg/errors from 0.8.1 to 0.9.0 Merge PR CosmWasm#253: Bump SDK master commit Merge PR CosmWasm#252: v2.0.4 Changelog Merge PR CosmWasm#249: docker image build and upload for release tags Merge PR CosmWasm#247: Update SDK Commit & Update CLI Doc Merge PR CosmWasm#246: Update Archive page with explorers Merge PR CosmWasm#245: archives Bump SDK commit to the latest master (CosmWasm#239) Merge PR CosmWasm#215: Update simulation tests Update join-mainnet.md (CosmWasm#229) Incorporate SDK's latest changes (CosmWasm#227) Merge PR CosmWasm#221: Hide unnecessarily exported function to better coverage report ...

Propose contract methods

f5f7758

alpe requested a review from ethanfrey August 20, 2020 13:33

ethanfrey reviewed Aug 20, 2020

View reviewed changes

alpe closed this Aug 21, 2020

alpe deleted the ibc_spec_contracts branch August 27, 2020 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propose contract methods #258

Propose contract methods #258

alpe commented Aug 20, 2020

codecov bot commented Aug 20, 2020

ethanfrey left a comment

ethanfrey Aug 20, 2020

ethanfrey Aug 20, 2020

ethanfrey Aug 20, 2020

ethanfrey Aug 20, 2020

ethanfrey Aug 20, 2020

ethanfrey Aug 20, 2020

ethanfrey Aug 20, 2020

ethanfrey Aug 20, 2020

ethanfrey Aug 20, 2020

ethanfrey Aug 20, 2020

ethanfrey Aug 20, 2020

alpe commented Aug 21, 2020

ethanfrey commented Aug 21, 2020

Propose contract methods #258

Propose contract methods #258

Conversation

alpe commented Aug 20, 2020

codecov bot commented Aug 20, 2020

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpe commented Aug 21, 2020

ethanfrey commented Aug 21, 2020