Skip to content

Commit

Permalink
Merge pull request #68 from cosmos/colin/1311-remove-channel-crossing…
Browse files Browse the repository at this point in the history
…-hellos

Continuation of removing crossing hellos in channel handshake
  • Loading branch information
vuong177 authored Jul 7, 2022
2 parents aa21130 + 86d61ed commit 4aa75e5
Show file tree
Hide file tree
Showing 16 changed files with 410 additions and 180 deletions.
130 changes: 130 additions & 0 deletions .github/scripts/build_test_matrix.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
package main

import (
"encoding/json"
"fmt"
"go/ast"
"go/parser"
"go/token"
"io/fs"
"os"
"path/filepath"
"strings"
)

const (
testNamePrefix = "Test"
testFileNameSuffix = "_test.go"
e2eTestDirectory = "e2e"
)

// GithubActionTestMatrix represents
type GithubActionTestMatrix struct {
Include []TestSuitePair `json:"include"`
}

type TestSuitePair struct {
Test string `json:"test"`
Suite string `json:"suite"`
}

func main() {
githubActionMatrix, err := getGithubActionMatrixForTests(e2eTestDirectory)
if err != nil {
fmt.Printf("error generating github action json: %s", err)
os.Exit(1)
}

ghBytes, err := json.Marshal(githubActionMatrix)
if err != nil {
fmt.Printf("error marshalling github action json: %s", err)
os.Exit(1)
}
fmt.Println(string(ghBytes))
}

// getGithubActionMatrixForTests returns a json string representing the contents that should go in the matrix
// field in a github action workflow. This string can be used with `fromJSON(str)` to dynamically build
// the workflow matrix to include all E2E tests under the e2eRootDirectory directory.
func getGithubActionMatrixForTests(e2eRootDirectory string) (GithubActionTestMatrix, error) {
testSuiteMapping := map[string][]string{}
fset := token.NewFileSet()
err := filepath.Walk(e2eRootDirectory, func(path string, info fs.FileInfo, err error) error {
// only look at test files
if !strings.HasSuffix(path, testFileNameSuffix) {
return nil
}

f, err := parser.ParseFile(fset, path, nil, 0)
if err != nil {
return fmt.Errorf("failed parsing file: %s", err)
}

suiteNameForFile, testCases, err := extractSuiteAndTestNames(f)
if err != nil {
return fmt.Errorf("failed extracting test suite name and test cases: %s", err)
}

testSuiteMapping[suiteNameForFile] = testCases

return nil
})

if err != nil {
return GithubActionTestMatrix{}, err
}

gh := GithubActionTestMatrix{
Include: []TestSuitePair{},
}

for testSuiteName, testCases := range testSuiteMapping {
for _, testCaseName := range testCases {
gh.Include = append(gh.Include, TestSuitePair{
Test: testCaseName,
Suite: testSuiteName,
})
}
}

return gh, nil
}

// extractSuiteAndTestNames extracts the name of the test suite function as well
// as all tests associated with it in the same file.
func extractSuiteAndTestNames(file *ast.File) (string, []string, error) {
var suiteNameForFile string
var testCases []string

for _, d := range file.Decls {
if f, ok := d.(*ast.FuncDecl); ok {
functionName := f.Name.Name
if isTestSuiteMethod(f) {
if suiteNameForFile != "" {
return "", nil, fmt.Errorf("found a second test function: %s when %s was already found", f.Name.Name, suiteNameForFile)
}
suiteNameForFile = functionName
continue
}
if isTestFunction(f) {
testCases = append(testCases, functionName)
}
}
}
if suiteNameForFile == "" {
return "", nil, fmt.Errorf("file %s had no test suite test case", file.Name.Name)
}
return suiteNameForFile, testCases, nil
}

// isTestSuiteMethod returns true if the function is a test suite function.
// e.g. func TestFeeMiddlewareTestSuite(t *testing.T) { ... }
func isTestSuiteMethod(f *ast.FuncDecl) bool {
return strings.HasPrefix(f.Name.Name, testNamePrefix) && len(f.Type.Params.List) == 1
}

// isTestFunction returns true if the function name starts with "Test" and has no parameters.
// as test suite functions do not accept a *testing.T.
func isTestFunction(f *ast.FuncDecl) bool {
return strings.HasPrefix(f.Name.Name, testNamePrefix) && len(f.Type.Params.List) == 0
}
162 changes: 162 additions & 0 deletions .github/scripts/build_test_matrix_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
package main

import (
"os"
"path"
"sort"
"strings"
"testing"

"github.com/stretchr/testify/assert"
)

const (
nonTestFile = "not_test_file.go"
goTestFileNameOne = "first_go_file_test.go"
goTestFileNameTwo = "second_go_file_test.go"
)

func TestGetGithubActionMatrixForTests(t *testing.T) {
t.Run("empty dir does not fail", func(t *testing.T) {
testingDir := t.TempDir()
_, err := getGithubActionMatrixForTests(testingDir)
assert.NoError(t, err)
})

t.Run("only test functions are picked up", func(t *testing.T) {
testingDir := t.TempDir()
createFileWithTestSuiteAndTests(t, "FeeMiddlewareTestSuite", "TestA", "TestB", testingDir, goTestFileNameOne)

gh, err := getGithubActionMatrixForTests(testingDir)
assert.NoError(t, err)

expected := GithubActionTestMatrix{
Include: []TestSuitePair{
{
Suite: "TestFeeMiddlewareTestSuite",
Test: "TestA",
},
{
Suite: "TestFeeMiddlewareTestSuite",
Test: "TestB",
},
},
}
assertGithubActionTestMatricesEqual(t, expected, gh)
})

t.Run("all files are picked up", func(t *testing.T) {
testingDir := t.TempDir()
createFileWithTestSuiteAndTests(t, "FeeMiddlewareTestSuite", "TestA", "TestB", testingDir, goTestFileNameOne)
createFileWithTestSuiteAndTests(t, "TransferTestSuite", "TestC", "TestD", testingDir, goTestFileNameTwo)

gh, err := getGithubActionMatrixForTests(testingDir)
assert.NoError(t, err)

expected := GithubActionTestMatrix{
Include: []TestSuitePair{
{
Suite: "TestTransferTestSuite",
Test: "TestC",
},
{
Suite: "TestFeeMiddlewareTestSuite",
Test: "TestA",
},
{
Suite: "TestFeeMiddlewareTestSuite",
Test: "TestB",
},
{
Suite: "TestTransferTestSuite",
Test: "TestD",
},
},
}

assertGithubActionTestMatricesEqual(t, expected, gh)
})

t.Run("non test files are not picked up", func(t *testing.T) {
testingDir := t.TempDir()
createFileWithTestSuiteAndTests(t, "FeeMiddlewareTestSuite", "TestA", "TestB", testingDir, nonTestFile)

gh, err := getGithubActionMatrixForTests(testingDir)
assert.NoError(t, err)
assert.Empty(t, gh.Include)
})

t.Run("fails when there are multiple suite runs", func(t *testing.T) {
testingDir := t.TempDir()
createFileWithTestSuiteAndTests(t, "FeeMiddlewareTestSuite", "TestA", "TestB", testingDir, nonTestFile)

fileWithTwoSuites := `package foo
func SuiteOne(t *testing.T) {
suite.Run(t, new(FeeMiddlewareTestSuite))
}
func SuiteTwo(t *testing.T) {
suite.Run(t, new(FeeMiddlewareTestSuite))
}
type FeeMiddlewareTestSuite struct {}
`

err := os.WriteFile(path.Join(testingDir, goTestFileNameOne), []byte(fileWithTwoSuites), os.FileMode(777))
assert.NoError(t, err)

_, err = getGithubActionMatrixForTests(testingDir)
assert.Error(t, err)
})
}

func assertGithubActionTestMatricesEqual(t *testing.T, expected, actual GithubActionTestMatrix) {
// sort by both suite and test as the order of the end result does not matter as
// all tests will be run.
sort.SliceStable(expected.Include, func(i, j int) bool {
memberI := expected.Include[i]
memberJ := expected.Include[j]
if memberI.Suite == memberJ.Suite {
return memberI.Test < memberJ.Test
}
return memberI.Suite < memberJ.Suite
})

sort.SliceStable(actual.Include, func(i, j int) bool {
memberI := actual.Include[i]
memberJ := actual.Include[j]
if memberI.Suite == memberJ.Suite {
return memberI.Test < memberJ.Test
}
return memberI.Suite < memberJ.Suite
})
assert.Equal(t, expected.Include, actual.Include)
}

func goTestFileContents(suiteName, fnName1, fnName2 string) string {

replacedSuiteName := strings.ReplaceAll(`package foo
func TestSuiteName(t *testing.T) {
suite.Run(t, new(SuiteName))
}
type SuiteName struct {}
func (s *SuiteName) fnName1() {}
func (s *SuiteName) fnName2() {}
func (s *SuiteName) suiteHelper() {}
func helper() {}
`, "SuiteName", suiteName)

replacedFn1Name := strings.ReplaceAll(replacedSuiteName, "fnName1", fnName1)
return strings.ReplaceAll(replacedFn1Name, "fnName2", fnName2)
}

func createFileWithTestSuiteAndTests(t *testing.T, suiteName, fn1Name, fn2Name, dir, filename string) {
goFileContents := goTestFileContents(suiteName, fn1Name, fn2Name)
err := os.WriteFile(path.Join(dir, filename), []byte(goFileContents), os.FileMode(777))
assert.NoError(t, err)
}
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ jobs:
steps:
- uses: actions/checkout@v3
- name: Create a file with all the pkgs
run: go list ./... > pkgs.txt
run: go list ./... ./.github/scripts > pkgs.txt
- name: Split pkgs into 4 files
run: split -d -n l/4 pkgs.txt pkgs.txt.part.
# cache multiple
Expand Down
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### API Breaking

* (modules/core/04-channel) [\#1317](https://github.com/cosmos/ibc-go/pull/1317) Remove crossing hellos from channel handshakes. The `PreviousChannelId` in `MsgChannelOpenTry` has been deprecated.
* (transfer) [\#1250](https://github.com/cosmos/ibc-go/pull/1250) Deprecate `GetTransferAccount` since the `transfer` module account is never used.
* (channel) [\#1283](https://github.com/cosmos/ibc-go/pull/1283) The `OnChanOpenInit` application callback now returns a version string in line with the latest [spec changes](https://github.com/cosmos/ibc/pull/629).
* (modules/29-fee)[\#1338](https://github.com/cosmos/ibc-go/pull/1338) Renaming `Result` field in `IncentivizedAcknowledgement` to `AppAcknowledgement`.
Expand All @@ -62,7 +63,6 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (modules/core/04-channel) [\#1232](https://github.com/cosmos/ibc-go/pull/1232) Updating params on `NewPacketId` and moving to bottom of file.
* (app/29-fee) [\#1305](https://github.com/cosmos/ibc-go/pull/1305) Change version string for fee module to `ics29-1`
* (app/29-fee) [\#1341](https://github.com/cosmos/ibc-go/pull/1341) Check if the fee module is locked and if the fee module is enabled before refunding all fees
* (modules/core/04-channel) [\#1317](https://github.com/cosmos/ibc-go/pull/1317) Remove crossing hellos from ChanOpenTry
* (transfer) [\#1414](https://github.com/cosmos/ibc-go/pull/1414) Emitting Sender address from `fungible_token_packet` events in `OnRecvPacket` and `OnAcknowledgementPacket`.
* (testing/simapp) [\#1397](https://github.com/cosmos/ibc-go/pull/1397) Adding mock module to maccperms and adding check to ensure mock module is not a blocked account address.
* (core/02-client) [\#1570](https://github.com/cosmos/ibc-go/pull/1570) Emitting an event when handling an upgrade client proposal.
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ view-docs:
test: test-unit
test-all: test-unit test-ledger-mock test-race test-cover

TEST_PACKAGES=./...
TEST_PACKAGES=./... ./.github/scripts
TEST_TARGETS := test-unit test-unit-amino test-unit-proto test-ledger-mock test-race test-ledger test-race

# Test runs-specific rules. To add a new test target, just add
Expand Down
12 changes: 3 additions & 9 deletions docs/ibc/apps.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,9 @@ OnChanOpenTry(
counterparty channeltypes.Counterparty,
counterpartyVersion string,
) (string, error) {
// Module may have already claimed capability in OnChanOpenInit in the case of crossing hellos
// (ie chainA and chainB both call ChanOpenInit before one of them calls ChanOpenTry)
// If the module can already authenticate the capability then the module already owns it so we don't need to claim
// Otherwise, module does not have channel capability and we must claim it from IBC
if !k.AuthenticateCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)) {
// Only claim channel capability passed back by IBC module if we do not already own it
if err := k.scopedKeeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
return err
}
// OpenTry must claim the channelCapability that IBC passes into the callback
if err := k.scopedKeeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
return err
}

// ... do custom initialization logic
Expand Down
2 changes: 1 addition & 1 deletion docs/ibc/proto-docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -2949,7 +2949,7 @@ value will be ignored by core IBC.
| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `port_id` | [string](#string) | | |
| `previous_channel_id` | [string](#string) | | in the case of crossing hello's, when both chains call OpenInit, we need the channel identifier of the previous channel in state INIT |
| `previous_channel_id` | [string](#string) | | **Deprecated.** Deprecated: this field is unused. Crossing hello's are no longer supported in core IBC. |
| `channel` | [Channel](#ibc.core.channel.v1.Channel) | | NOTE: the version field within the channel has been deprecated. Its value will be ignored by core IBC. |
| `counterparty_version` | [string](#string) | | |
| `proof_init` | [bytes](#bytes) | | |
Expand Down
10 changes: 10 additions & 0 deletions docs/migrations/v3-to-v4.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ No genesis or in-place migrations required when upgrading from v1 or v2 of ibc-g

## Chains

- No relevant changes were made in this release.

## IBC Apps

### ICS04 - Channel

The `WriteAcknowledgement` API now takes the `exported.Acknowledgement` type instead of passing in the acknowledgement byte array directly.
Expand All @@ -30,6 +34,10 @@ The `NewErrorAcknowledgement` method signature has changed.
It now accepts an `error` rather than a `string`. This was done in order to prevent accidental state changes.
All error acknowledgements now contain a deterministic ABCI code and error message. It is the responsibility of the application developer to emit error details in events.

Crossing hellos have been removed from 04-channel handshake negotiation.
IBC Applications no longer need to account from already claimed capabilities in the `OnChanOpenTry` callback. The capability provided by core IBC must be able to be claimed with error.
`PreviousChannelId` in `MsgChannelOpenTry` has been deprecated and is no longer used by core IBC.

### ICS27 - Interchain Accounts

The `RegisterInterchainAccount` API has been modified to include an additional `version` argument. This change has been made in order to support ICS29 fee middleware, for relayer incentivization of ICS27 packets.
Expand Down Expand Up @@ -91,3 +99,5 @@ if err := k.icaControllerKeeper.RegisterInterchainAccount(ctx, msg.ConnectionId,
## Relayers

When using the `DenomTrace` gRPC, the full IBC denomination with the `ibc/` prefix may now be passed in.

Crossing hellos are no longer supported by core IBC. The handshake should be completed in the logical 4 step process (INIT, TRY, ACK, CONFIRM).
Loading

0 comments on commit 4aa75e5

Please sign in to comment.