Continuation of removing crossing hellos in channel handshake

.github/scripts/build_test_matrix.go

@@ -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
+}

.github/scripts/build_test_matrix_test.go

@@ -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)
+}

.github/workflows/test.yml

@@ -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

CHANGELOG.md

@@ -62,7 +62,7 @@ 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
+* (modules/core/04-channel) [\#1317](https://github.com/cosmos/ibc-go/pull/1317) Remove corssing hellos from channel handshakes. The `PreviousChannelId` in `MsgChannelOpenTry` has been deprecated.  
 * (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. 

Makefile

@@ -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

docs/ibc/apps.md

@@ -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

docs/ibc/proto-docs.md

@@ -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) |  |  |

docs/migrations/v3-to-v4.md

@@ -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. 
@@ -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.
@@ -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 hello's are no longer supported by core IBC. The handshake should be completed in the logical 4 step process (INIT, TRY, ACK, CONFIRM).