Repository: onflow/flow-cli
Pull request #1607 (2024-05-17 20:25:48)

Handle transaction result errors in `flow migrate stage`
## Description

Errors while staging a contract (related to a failed transaction) are not properly reported.  This handles that case.

______

For contributor use:

- [ ] Targeted PR against `master` branch
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work
- [x] Code follows the [standards mentioned here](https://github.com/onflow/flow-cli/blob/master/CONTRIBUTING.md#styleguides)
- [ ] Updated relevant documentation
- [x] Re-reviewed `Files changed` in the Github PR explorer
- [x] Added appropriate labels


codecov-commenter: ## [Codecov](https://app.codecov.io/gh/onflow/flow-cli/pull/1607?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onflow) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 42.17%. Comparing base [(`ad513f5`)](https://app.codecov.io/gh/onflow/flow-cli/commit/ad513f5ef53bc5929f6c98b13de7ce9b117c784b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onflow) to head [(`401fb13`)](https://app.codecov.io/gh/onflow/flow-cli/pull/1607?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onflow).


<details><summary>Additional details and impacted files</summary>


```diff
@@                    Coverage Diff                     @@
##           feature/stable-cadence    #1607      +/-   ##
==========================================================
+ Coverage                   42.02%   42.17%   +0.14%     
==========================================================
  Files                          60       60              
  Lines                        4200     4202       +2     
==========================================================
+ Hits                         1765     1772       +7     
+ Misses                       2200     2197       -3     
+ Partials                      235      233       -2     
```

| [Flag](https://app.codecov.io/gh/onflow/flow-cli/pull/1607/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onflow) | Coverage Δ | |
|---|---|---|
| [unittests](https://app.codecov.io/gh/onflow/flow-cli/pull/1607/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onflow) | `42.17% <100.00%> (+0.14%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onflow#carryforward-flags-in-the-pull-request-comment) to find out more.


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/onflow/flow-cli/pull/1607?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onflow).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onflow).


Patch:
diff --git a/internal/migrate/staging_service.go b/internal/migrate/staging_service.go
index 294d4a8d..f62d9625 100644
--- a/internal/migrate/staging_service.go
+++ b/internal/migrate/staging_service.go
@@ -211,7 +211,7 @@ func (s *stagingServiceImpl) stageContract(ctx context.Context, contract stagedC
 		return flow.Identifier{}, fmt.Errorf("failed to get account for contract %s: %w", contract.DeployLocation.Name, err)
 	}
 
-	tx, _, err := s.flow.SendTransaction(
+	_, res, err := s.flow.SendTransaction(
 		context.Background(),
 		transactions.SingleAccountRole(*account),
 		flowkit.Script{
@@ -224,7 +224,11 @@ func (s *stagingServiceImpl) stageContract(ctx context.Context, contract stagedC
 		return flow.Identifier{}, err
 	}
 
-	return tx.ID(), nil
+	if res.Error != nil {
+		return res.TransactionID, fmt.Errorf("failed to stage contract: %w", res.Error)
+	}
+
+	return res.TransactionID, nil
 }
 
 func (s *stagingServiceImpl) hasStagedContractChanged(contract stagedContractUpdate) bool {


Test patch:
diff --git a/internal/migrate/staging_service_test.go b/internal/migrate/staging_service_test.go
index 1ee22d3f..a212bb26 100644
--- a/internal/migrate/staging_service_test.go
+++ b/internal/migrate/staging_service_test.go
@@ -21,6 +21,7 @@ package migrate
 import (
 	"context"
 	"errors"
+	"fmt"
 	"path/filepath"
 	"reflect"
 	"testing"
@@ -108,6 +109,7 @@ func Test_StagingService(t *testing.T) {
 	setupMocks := func(
 		accts []mockAccount,
 		mocksStagedContracts map[common.AddressLocation][]byte,
+		txResult *flow.TransactionResult,
 	) (*flowkitMocks.Services, *flowkit.State, []*project.Contract) {
 		srv := flowkitMocks.NewServices(t)
 		rw, _ := tests.ReaderWriter()
@@ -144,7 +146,7 @@ func Test_StagingService(t *testing.T) {
 
 			_, ok = script.Args[1].(cadence.String)
 			return ok
-		}), mock.Anything).Return(tests.NewTransaction(), nil, nil).Maybe()
+		}), mock.Anything).Return(tests.NewTransaction(), txResult, nil).Maybe()
 
 		// Mock staged contracts on network
 		for location, code := range mocksStagedContracts {
@@ -212,7 +214,7 @@ func Test_StagingService(t *testing.T) {
 				},
 			},
 		}
-		srv, state, deploymentContracts := setupMocks(mockAccount, nil)
+		srv, state, deploymentContracts := setupMocks(mockAccount, nil, tests.NewTransactionResult(nil))
 
 		v := newMockStagingValidator(t)
 		v.On("Validate", mock.MatchedBy(func(stagedContracts []stagedContractUpdate) bool {
@@ -275,7 +277,7 @@ func Test_StagingService(t *testing.T) {
 				},
 			},
 		}
-		srv, state, deploymentContracts := setupMocks(mockAccount, nil)
+		srv, state, deploymentContracts := setupMocks(mockAccount, nil, tests.NewTransactionResult(nil))
 
 		v := newMockStagingValidator(t)
 		v.On("Validate", mock.MatchedBy(func(stagedContracts []stagedContractUpdate) bool {
@@ -337,7 +339,7 @@ func Test_StagingService(t *testing.T) {
 				},
 			},
 		}
-		srv, state, deploymentContracts := setupMocks(mockAccount, nil)
+		srv, state, deploymentContracts := setupMocks(mockAccount, nil, tests.NewTransactionResult(nil))
 
 		s := newStagingService(
 			srv,
@@ -380,7 +382,7 @@ func Test_StagingService(t *testing.T) {
 				},
 			},
 		}
-		srv, state, deploymentContracts := setupMocks(mockAccount, nil)
+		srv, state, deploymentContracts := setupMocks(mockAccount, nil, tests.NewTransactionResult(nil))
 
 		v := newMockStagingValidator(t)
 		v.On("Validate", mock.MatchedBy(func(stagedContracts []stagedContractUpdate) bool {
@@ -449,7 +451,7 @@ func Test_StagingService(t *testing.T) {
 				},
 			},
 		}
-		srv, state, deploymentContracts := setupMocks(mockAccount, nil)
+		srv, state, deploymentContracts := setupMocks(mockAccount, nil, tests.NewTransactionResult(nil))
 
 		v := newMockStagingValidator(t)
 		v.On("Validate", mock.MatchedBy(func(stagedContracts []stagedContractUpdate) bool {
@@ -517,7 +519,7 @@ func Test_StagingService(t *testing.T) {
 		}
 		srv, state, deploymentContracts := setupMocks(mockAccount, map[common.AddressLocation][]byte{
 			simpleAddressLocation("0x01.Foo"): []byte("access(all) contract Foo {}"),
-		})
+		}, tests.NewTransactionResult(nil))
 
 		v := newMockStagingValidator(t)
 		v.On("Validate", mock.MatchedBy(func(stagedContracts []stagedContractUpdate) bool {
@@ -556,4 +558,61 @@ func Test_StagingService(t *testing.T) {
 
 		srv.AssertNumberOfCalls(t, "SendTransaction", 0)
 	})
+
+	t.Run("handles error transaction result", func(t *testing.T) {
+		mockAccount := []mockAccount{
+			{
+				name:    "some-account",
+				address: "0x01",
+				deployments: []mockDeployment{
+					{
+						name: "Foo",
+						code: "access(all) contract Foo {}",
+					},
+				},
+			},
+		}
+		srv, state, deploymentContracts := setupMocks(mockAccount, nil, &flow.TransactionResult{
+			Status:        flow.TransactionStatusSealed,
+			Error:         fmt.Errorf("i am a transaction error"),
+			TransactionID: flow.Identifier{0x99},
+		})
+
+		v := newMockStagingValidator(t)
+		v.On("Validate", mock.MatchedBy(func(stagedContracts []stagedContractUpdate) bool {
+			return reflect.DeepEqual(stagedContracts, []stagedContractUpdate{
+				{
+					DeployLocation: simpleAddressLocation("0x01.Foo"),
+					SourceLocation: common.StringLocation(filepath.FromSlash("0x01/Foo.cdc")),
+					Code:           []byte("access(all) contract Foo {}"),
+				},
+			})
+		})).Return(nil).Once()
+
+		s := newStagingService(
+			srv,
+			state,
+			util.NoLogger,
+			v,
+			func(sve *stagingValidatorError) bool {
+				return false
+			},
+		)
+
+		results, err := s.StageContracts(
+			context.Background(),
+			deploymentContracts,
+		)
+
+		require.NoError(t, err)
+		require.NotNil(t, results)
+
+		require.Equal(t, 1, len(results))
+		require.Contains(t, results, simpleAddressLocation("0x01.Foo"))
+		require.ErrorContains(t, results[simpleAddressLocation("0x01.Foo")].Err, "i am a transaction error")
+		require.Equal(t, results[simpleAddressLocation("0x01.Foo")].WasValidated, true)
+		require.Equal(t, flow.Identifier{0x99}, results[simpleAddressLocation("0x01.Foo")].TxId)
+
+		srv.AssertNumberOfCalls(t, "SendTransaction", 1)
+	})
 }