fix(execute): reconcile writes that report completed without a transaction hash

cmd/execute/contract_call.go

@@ -124,7 +124,7 @@ func NewContractCallCmd(f *cmdutil.Factory) *cobra.Command {
 					})
 				}
 
-				if execTerminalStatuses[writeResp.Status] {
+				if execTerminalStatuses[writeResp.Status] && !completedWithoutTransaction(writeResp.Status, writeResp.TransactionHash) {
 					if err := terminalExecError(writeResp.ExecutionID, writeResp.Status, nil); err != nil {
 						return err
 					}

cmd/execute/pollhint_internal_test.go

@@ -0,0 +1,86 @@
+package execute
+
+import (
+	"net/http"
+	"testing"
+	"time"
+)
+
+// A successful /api/execute/contract-call broadcast can return 202 with status "completed" and
+// no transactionHash; the hash only appears on the status endpoint, so a direct-write response in
+// this shape is worth one reconciling fetch. On a status response the same shape is legitimate,
+// because an action that submits nothing onchain completes this way, so it is only reported.
+func TestCompletedWithoutTransaction(t *testing.T) {
+	empty := ""
+	hash := "0xabc"
+
+	cases := []struct {
+		name   string
+		status string
+		tx     *string
+		want   bool
+	}{
+		{"completed without a hash", "completed", nil, true},
+		{"completed with an empty hash", "completed", &empty, true},
+		{"completed with a hash", "completed", &hash, false},
+		{"failed carries no such expectation", "failed", nil, false},
+		{"running is not completed", "running", nil, false},
+		{"unconfirmed already implies a broadcast", "unconfirmed", nil, false},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := completedWithoutTransaction(tc.status, tc.tx); got != tc.want {
+				t.Errorf("completedWithoutTransaction(%q, %v) = %v, want %v", tc.status, tc.tx, got, tc.want)
+			}
+		})
+	}
+}
+
+// unconfirmed is terminal for a client: nothing moves it until the reconciler runs, so a poll loop
+// that treats it as pending just burns requests until the caller's timeout.
+func TestExecTerminalStatuses(t *testing.T) {
+	for _, status := range []string{"completed", "failed", "unconfirmed"} {
+		if !execTerminalStatuses[status] {
+			t.Errorf("expected %q to be terminal", status)
+		}
+	}
+	for _, status := range []string{"pending", "running"} {
+		if execTerminalStatuses[status] {
+			t.Errorf("expected %q not to be terminal", status)
+		}
+	}
+}
+
+func TestNextPollDelay(t *testing.T) {
+	cases := []struct {
+		name         string
+		header       string
+		setHeader    bool
+		wantDelay    time.Duration
+		wantTerminal bool
+	}{
+		{"no header falls back to the default", "", false, defaultPollInterval, false},
+		{"a hint is honoured", "5", true, 5 * time.Second, false},
+		{"a hint of zero means terminal", "0", true, 0, true},
+		{"surrounding whitespace is tolerated", "  3 ", true, 3 * time.Second, false},
+		{"an unparseable hint falls back rather than failing", "soon", true, defaultPollInterval, false},
+		{"a negative hint falls back", "-1", true, defaultPollInterval, false},
+		{"a hint at the ceiling is honoured", "30", true, maxPollIntervalSecs * time.Second, false},
+		{"an oversized hint is clamped to the ceiling", "3600", true, maxPollIntervalSecs * time.Second, false},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			resp := &http.Response{Header: http.Header{}}
+			if tc.setHeader {
+				resp.Header.Set("X-Poll-Interval-Hint", tc.header)
+			}
+			delay, terminal := nextPollDelay(resp)
+			if delay != tc.wantDelay || terminal != tc.wantTerminal {
+				t.Errorf("nextPollDelay(%q) = (%v, %v), want (%v, %v)",
+					tc.header, delay, terminal, tc.wantDelay, tc.wantTerminal)
+			}
+		})
+	}
+}

cmd/execute/reconcile_test.go

@@ -0,0 +1,256 @@
+package execute_test
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/keeperhub/cli/cmd/execute"
+	"github.com/keeperhub/cli/pkg/iostreams"
+)
+
+// The regression this addresses. A write reports completed with no transaction hash, which the
+// live API does for /api/execute/contract-call, so --wait must reconcile against the status
+// endpoint instead of returning a success the caller cannot verify.
+func TestTransferCmd_WaitReconcilesCompletedWithoutHash(t *testing.T) {
+	statusCalls := 0
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		if strings.HasSuffix(r.URL.Path, "/status") {
+			statusCalls++
+			w.Header().Set("X-Poll-Interval-Hint", "0")
+			_, _ = w.Write([]byte(`{"executionId":"exec-1","status":"completed","transactionHash":"0xdeadbeef"}`))
+			return
+		}
+		w.WriteHeader(http.StatusAccepted)
+		_, _ = w.Write([]byte(`{"executionId":"exec-1","status":"completed"}`))
+	}))
+	defer srv.Close()
+
+	ios, buf, _, _ := iostreams.Test()
+	cmd := execute.NewTransferCmd(newTransferFactory(ios, srv))
+	cmd.SetArgs([]string{"--chain", "84532", "--to", "0xabc", "--amount", "0.1", "--wait"})
+
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if statusCalls == 0 {
+		t.Fatal("expected the status endpoint to be polled so the caller ends up with a transaction hash")
+	}
+	if out := buf.String(); !strings.Contains(out, "0xdeadbeef") {
+		t.Errorf("expected the reconciled transaction hash in the output, got: %q", out)
+	}
+}
+
+func TestContractCallCmd_WaitReconcilesCompletedWithoutHash(t *testing.T) {
+	statusCalls := 0
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		if strings.HasSuffix(r.URL.Path, "/status") {
+			statusCalls++
+			w.Header().Set("X-Poll-Interval-Hint", "0")
+			_, _ = w.Write([]byte(`{"executionId":"exec-cc","status":"completed","transactionHash":"0xc0ffee"}`))
+			return
+		}
+		w.WriteHeader(http.StatusAccepted)
+		_, _ = w.Write([]byte(`{"executionId":"exec-cc","status":"completed"}`))
+	}))
+	defer srv.Close()
+
+	ios, buf, _, _ := iostreams.Test()
+	cmd := execute.NewContractCallCmd(newContractCallFactory(ios, srv))
+	cmd.SetArgs([]string{
+		"--chain", "84532",
+		"--contract", "0x2A6FC8182Bf9928Ef7517dA980dC79e8107c555A",
+		"--method", "ping",
+		"--wait",
+	})
+
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if statusCalls == 0 {
+		t.Fatal("expected the status endpoint to be polled for a completed write with no hash")
+	}
+	if out := buf.String(); !strings.Contains(out, "0xc0ffee") {
+		t.Errorf("expected the reconciled transaction hash in the output, got: %q", out)
+	}
+}
+
+// completed with no transaction is legitimate: a read-only call or a non-transaction step
+// completes without submitting anything. The three paths that consume a status response all
+// report the condition and all exit zero. Erroring on it is opt-in behaviour, not the default.
+func TestTransferCmd_WaitReportsCompletedWithoutTransaction(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		if strings.HasSuffix(r.URL.Path, "/status") {
+			w.Header().Set("X-Poll-Interval-Hint", "0")
+			_, _ = w.Write([]byte(`{"executionId":"exec-noop","status":"completed"}`))
+			return
+		}
+		w.WriteHeader(http.StatusAccepted)
+		_, _ = w.Write([]byte(`{"executionId":"exec-noop","status":"completed"}`))
+	}))
+	defer srv.Close()
+
+	ios, buf, _, _ := iostreams.Test()
+	cmd := execute.NewTransferCmd(newTransferFactory(ios, srv))
+	cmd.SetArgs([]string{"--chain", "84532", "--to", "0xabc", "--amount", "0.1", "--wait"})
+
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("expected a completed execution with no transaction to succeed, got: %v", err)
+	}
+	if out := buf.String(); !strings.Contains(out, "none submitted") {
+		t.Errorf("expected the output to report that nothing was submitted, got: %q", out)
+	}
+}
+
+func TestExecStatusCmd_ReportsCompletedWithoutTransaction(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"executionId":"exec-noop","status":"completed","type":"contract-call"}`))
+	}))
+	defer srv.Close()
+
+	ios, buf, _, _ := iostreams.Test()
+	cmd := execute.NewStatusCmd(newStatusFactory(ios, srv))
+	cmd.SetArgs([]string{"exec-noop"})
+
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("expected a completed execution with no transaction to succeed, got: %v", err)
+	}
+	if out := buf.String(); !strings.Contains(out, "none submitted") {
+		t.Errorf("expected the output to report that nothing was submitted, got: %q", out)
+	}
+}
+
+func TestExecStatusCmd_WatchReportsCompletedWithoutTransaction(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.Header().Set("X-Poll-Interval-Hint", "0")
+		_, _ = w.Write([]byte(`{"executionId":"exec-noop","status":"completed"}`))
+	}))
+	defer srv.Close()
+
+	ios, buf, _, _ := iostreams.Test()
+	cmd := execute.NewStatusCmd(newStatusFactory(ios, srv))
+	cmd.SetArgs([]string{"exec-noop", "--watch"})
+
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("expected a completed execution with no transaction to succeed, got: %v", err)
+	}
+	if out := buf.String(); !strings.Contains(out, "none submitted") {
+		t.Errorf("expected the output to report that nothing was submitted, got: %q", out)
+	}
+}
+
+// unconfirmed is terminal. Nothing moves it until the reconciler runs on its own schedule, so a
+// poll loop that keeps going only burns requests until the caller's timeout. It exits zero, since
+// a non-zero exit invites a retry that can re-broadcast a transaction already onchain.
+func TestTransferCmd_WaitStopsOnUnconfirmedWithPollHint(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		if strings.HasSuffix(r.URL.Path, "/status") {
+			w.Header().Set("X-Poll-Interval-Hint", "1")
+			_, _ = w.Write([]byte(`{"executionId":"exec-unc","status":"unconfirmed","transactionHash":"0xunc"}`))
+			return
+		}
+		w.WriteHeader(http.StatusAccepted)
+		_, _ = w.Write([]byte(`{"executionId":"exec-unc","status":"pending"}`))
+	}))
+	defer srv.Close()
+
+	ios, buf, _, _ := iostreams.Test()
+	cmd := execute.NewTransferCmd(newTransferFactory(ios, srv))
+	cmd.SetArgs([]string{"--chain", "84532", "--to", "0xabc", "--amount", "0.1", "--wait", "--timeout", "3s"})
+
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("expected --wait to exit zero on unconfirmed, got: %v", err)
+	}
+	out := buf.String()
+	if !strings.Contains(out, "unconfirmed") {
+		t.Errorf("expected the unconfirmed status in the output, got: %q", out)
+	}
+	if !strings.Contains(out, "0xunc") {
+		t.Errorf("expected the transaction hash in the output, got: %q", out)
+	}
+}
+
+// If unconfirmed were treated as pending, --watch has no deadline to fall back on and this would
+// poll until the test binary is killed.
+func TestExecStatusCmd_WatchStopsOnUnconfirmed(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.Header().Set("X-Poll-Interval-Hint", "1")
+		_, _ = w.Write([]byte(`{"executionId":"exec-unc","status":"unconfirmed","transactionHash":"0xunc"}`))
+	}))
+	defer srv.Close()
+
+	ios, buf, _, _ := iostreams.Test()
+	cmd := execute.NewStatusCmd(newStatusFactory(ios, srv))
+	cmd.SetArgs([]string{"exec-unc", "--watch"})
+
+	done := make(chan error, 1)
+	go func() { done <- cmd.Execute() }()
+
+	select {
+	case err := <-done:
+		if err != nil {
+			t.Fatalf("expected --watch to exit zero on unconfirmed, got: %v", err)
+		}
+	case <-time.After(10 * time.Second):
+		t.Fatal("--watch kept polling an unconfirmed execution instead of stopping")
+	}
+
+	if out := buf.String(); !strings.Contains(out, "0xunc") {
+		t.Errorf("expected the transaction hash in the output, got: %q", out)
+	}
+}
+
+// The server's pacing is honoured rather than a fixed client-side timer. A large hint on the
+// first poll would previously have been ignored, and the client would have polled on its own
+// two second cadence regardless of what the server asked for.
+func TestPollHonoursServerInterval(t *testing.T) {
+	var firstPollAt, secondPollAt time.Time
+	polls := 0
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		if !strings.HasSuffix(r.URL.Path, "/status") {
+			w.WriteHeader(http.StatusAccepted)
+			_, _ = w.Write([]byte(`{"executionId":"exec-slow","status":"running"}`))
+			return
+		}
+		polls++
+		switch polls {
+		case 1:
+			firstPollAt = time.Now()
+			w.Header().Set("X-Poll-Interval-Hint", "1")
+			_, _ = w.Write([]byte(`{"executionId":"exec-slow","status":"running"}`))
+		default:
+			secondPollAt = time.Now()
+			w.Header().Set("X-Poll-Interval-Hint", "0")
+			_, _ = w.Write([]byte(`{"executionId":"exec-slow","status":"completed","transactionHash":"0xok"}`))
+		}
+	}))
+	defer srv.Close()
+
+	ios, _, _, _ := iostreams.Test()
+	cmd := execute.NewTransferCmd(newTransferFactory(ios, srv))
+	cmd.SetArgs([]string{"--chain", "84532", "--to", "0xabc", "--amount", "0.1", "--wait"})
+
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if polls < 2 {
+		t.Fatalf("expected at least two polls, got %d", polls)
+	}
+	gap := secondPollAt.Sub(firstPollAt)
+	if gap < 900*time.Millisecond {
+		t.Errorf("second poll came after %v, expected roughly the 1s the server asked for", gap)
+	}
+	if gap > 1900*time.Millisecond {
+		t.Errorf("second poll came after %v, which suggests the old fixed 2s timer rather than the hint", gap)
+	}
+}