Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logic for claiming ibc ports in x/wasm #211

Closed
wants to merge 14 commits into from

Conversation

ethanfrey
Copy link
Member

@ethanfrey ethanfrey commented Jul 20, 2020

This is the beginning of adding ibc support to wasmd.
It branches off our long-lived "stargate tracking" branch.

Check the new IBC docs before working on this.

Tasks:

  • Dynamically bind ports on contract creation
  • Implement ping/ping protocol that returns contract label when calling it's port
  • Implement 2nd simple protocol (TODO: define)
  • Look at connection protocol negotation
  • Fake sending messages from a contract and handling callbacks (go message to trigger these)

Requirements from ICF grant (should be covered by the above):

  • Integrate x/wasm module with Cosmos-SDK IBC implementation
  • Provide stubs for contracts in Go to test following:
  • Test dynamic port allocation (upon contract creation) and upstream any needed changes to Cosmos SDK
  • Test dynamic protocol creation (packets format defined after binary created) and upstream any needed changes to Cosmos SDK
  • Test protocol negotiation (eg. contract may speak ICS20 and a more advanced protocol, and find what matches the other side)
  • and upstream any needed changes to ICS spec and Cosmos SDK

At the end, we should have a mock of what a contract can do with IBC and figure out if the API provides all we need, but not touch the rust code at all - that is another (bigger) task.

Also:

  • Update to latest (stable) cosmos-sdk:master when appropriate

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added relevant godoc comments.

  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md

  • Re-reviewed Files changed in the Github PR explorer


For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Jul 20, 2020

Codecov Report

Merging #211 into 0.9.1_to_cosmos-sdk-0.39-master will decrease coverage by 5.08%.
The diff coverage is 8.79%.

Impacted file tree graph

@@                         Coverage Diff                         @@
##           0.9.1_to_cosmos-sdk-0.39-master     #211      +/-   ##
===================================================================
- Coverage                            32.42%   27.34%   -5.09%     
===================================================================
  Files                                   24       27       +3     
  Lines                                 3830     4739     +909     
===================================================================
+ Hits                                  1242     1296      +54     
- Misses                                2521     3369     +848     
- Partials                                67       74       +7     
Impacted Files Coverage Δ
app/integration/test_common.go 0.00% <0.00%> (ø)
x/wasm/handler.go 49.60% <0.00%> (-4.71%) ⬇️
x/wasm/internal/types/msg.go 37.16% <0.00%> (-3.61%) ⬇️
x/wasm/internal/types/types.pb.go 0.51% <1.51%> (-0.04%) ⬇️
x/wasm/ibc.go 6.06% <6.06%> (ø)
app/export.go 7.59% <9.09%> (ø)
x/wasm/internal/keeper/ibc.go 30.43% <30.43%> (ø)
x/wasm/internal/keeper/keeper.go 90.15% <75.00%> (-0.39%) ⬇️
app/app.go 92.05% <100.00%> (+0.22%) ⬆️
app/encoding.go 100.00% <100.00%> (ø)
... and 5 more

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 cc37320...b42e003. Read the comment docs.

@ethanfrey
Copy link
Member Author

ethanfrey commented Jul 21, 2020

@alpe I ported a lot of test setup code for IBC stuff from the sdk and finally tried to instantiate a contract and bind a port. Which fails:

go test ./x/wasm/internal/keeper -v -run TestBindingPortOnInstantiate

--- FAIL: TestBindingPortOnInstantiate (0.19s)
panic: identifier wasm:cosmos18vd8fpwxzck93qlwghaj6arh4p7c5n89uzcee5 has invalid length: 50, must be between 2-20 characters: invalid identifier [recovered]
        panic: identifier wasm:cosmos18vd8fpwxzck93qlwghaj6arh4p7c5n89uzcee5 has invalid length: 50, must be between 2-20 characters: invalid identifier

So, my naive port naming was simply to use the full contract address, which is well-known and unique and easy to map back and forth. This doesn't work 😞 We need to think of another naming scheme for the ports.

I could make an auto-sequence, but as this is the identifier when sending messages to the other chain, humans should recognize the connection. Take last 14 chars of the address???

You can comment out the test if you want and focus on your other work, but I wanted to make this visible... first semi-blocker.

(I asked for feedback on the ibc slack channel)

contractID := codeID<<32 + instanceID // as in contractAddress
size := binary.PutUvarint(data, contractID)
// max total length = 4 + 16
return portIDPrefix + base64.StdEncoding.WithPadding(base64.NoPadding).EncodeToString(data[0:size]) // encoded to make it readable
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit ugly but it works

}

func ContractFromPortID(portID string) (sdk.AccAddress, error) {
if !strings.HasPrefix(portID, portIDPrefix) {
return nil, sdkerrors.Wrapf(types.ErrInvalid, "without prefix")
}
return sdk.AccAddressFromBech32(portID[len(portIDPrefix):])
data, err := base64.StdEncoding.WithPadding(base64.NoPadding).DecodeString(portID[len(portIDPrefix):])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to have a test that these reverse properly (port <-> contract)

@ethanfrey
Copy link
Member Author

Closing and now included in Alex's IBC Spikes

@ethanfrey ethanfrey closed this Aug 18, 2020
@alpe alpe deleted the ibc-prototype branch August 27, 2020 09:57
zemyblue pushed a commit to Finschia/wasmd that referenced this pull request Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants