From a2b4a1d31ce62cdaa335de8282d21617d3a69983 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20C=2E=20Morency?= <1102868+fmorency@users.noreply.github.com> Date: Mon, 18 Mar 2024 16:01:01 -0400 Subject: [PATCH] fix: todos (#5) - DRY - Configurable tx and block timeouts --- cmd/claim.go | 14 ++--- cmd/common.go | 47 +++++++++++++++++ cmd/migrate.go | 61 ++++++++++++++-------- cmd/verify.go | 14 ++--- interchaintest/migrate_on_chain_test.go | 1 - {cmd => internal/config}/config.go | 68 +++++++------------------ internal/manifest/migrate.go | 42 +++++---------- internal/store/claim_test.go | 1 - testutils/httpresponder.go | 5 +- 9 files changed, 135 insertions(+), 118 deletions(-) rename {cmd => internal/config}/config.go (50%) diff --git a/cmd/claim.go b/cmd/claim.go index 06b973a..d954859 100644 --- a/cmd/claim.go +++ b/cmd/claim.go @@ -28,27 +28,27 @@ Trying to claim a work item that is already failed should return an error, unles } func ClaimCmdRunE(cmd *cobra.Command, args []string) error { - config := LoadConfigFromCLI("claim-uuid") - slog.Debug("args", "config", config) - if err := config.Validate(); err != nil { + c := LoadConfigFromCLI("claim-uuid") + slog.Debug("args", "c", c) + if err := c.Validate(); err != nil { return err } claimConfig := LoadClaimConfigFromCLI() - slog.Debug("args", "claim-config", claimConfig) + slog.Debug("args", "claim-c", claimConfig) authConfig := LoadAuthConfigFromCLI() - slog.Debug("args", "auth-config", authConfig) + slog.Debug("args", "auth-c", authConfig) if err := authConfig.Validate(); err != nil { return err } - r := CreateRestClient(cmd.Context(), config.Url, config.Neighborhood) + r := CreateRestClient(cmd.Context(), c.Url, c.Neighborhood) if err := AuthenticateRestClient(r, authConfig.Username, authConfig.Password); err != nil { return err } - item, err := claimWorkItem(r, config.UUID, claimConfig.Force) + item, err := claimWorkItem(r, c.UUID, claimConfig.Force) if err != nil { return err } diff --git a/cmd/common.go b/cmd/common.go index 31949c2..53f2dd4 100644 --- a/cmd/common.go +++ b/cmd/common.go @@ -9,6 +9,10 @@ import ( "github.com/go-resty/resty/v2" "github.com/pkg/errors" + "github.com/spf13/viper" + + "github.com/liftedinit/mfx-migrator/internal/config" + "github.com/liftedinit/mfx-migrator/internal/utils" "github.com/liftedinit/mfx-migrator/internal/store" ) @@ -70,3 +74,46 @@ func AuthenticateRestClient(r *resty.Client, username, password string) error { return nil } + +// LoadConfigFromCLI loads the Config from the CLI flags +// +// `uuidKey` is the name of the viper key that holds the UUID +// This is necessary because the UUID is used in multiple commands +func LoadConfigFromCLI(uuidKey string) config.Config { + return config.Config{ + UUID: viper.GetString(uuidKey), + Url: viper.GetString("url"), + Neighborhood: viper.GetUint64("neighborhood"), + } +} + +func LoadAuthConfigFromCLI() config.AuthConfig { + return config.AuthConfig{ + Username: viper.GetString("username"), + Password: viper.GetString("password"), + } +} + +func LoadClaimConfigFromCLI() config.ClaimConfig { + return config.ClaimConfig{ + Force: viper.GetBool("force"), + } +} + +func LoadMigrationConfigFromCLI() config.MigrateConfig { + var tokenMap map[string]utils.TokenInfo + if err := viper.UnmarshalKey("token-map", &tokenMap); err != nil { + panic(err) + } + return config.MigrateConfig{ + ChainID: viper.GetString("chain-id"), + AddressPrefix: viper.GetString("address-prefix"), + NodeAddress: viper.GetString("node-address"), + KeyringBackend: viper.GetString("keyring-backend"), + BankAddress: viper.GetString("bank-address"), + ChainHome: viper.GetString("chain-home"), + TokenMap: tokenMap, + WaitTxTimeout: viper.GetUint("wait-for-tx-timeout"), + WaitBlockTimeout: viper.GetUint("wait-for-block-timeout"), + } +} diff --git a/cmd/migrate.go b/cmd/migrate.go index 5559358..7a9d672 100644 --- a/cmd/migrate.go +++ b/cmd/migrate.go @@ -12,6 +12,8 @@ import ( "github.com/spf13/cobra" "github.com/spf13/viper" + "github.com/liftedinit/mfx-migrator/internal/config" + "github.com/liftedinit/mfx-migrator/internal/many" "github.com/liftedinit/mfx-migrator/internal/utils" @@ -27,26 +29,26 @@ var migrateCmd = &cobra.Command{ } func MigrateCmdRunE(cmd *cobra.Command, args []string) error { - config := LoadConfigFromCLI("migrate-uuid") - slog.Debug("args", "config", config) - if err := config.Validate(); err != nil { + c := LoadConfigFromCLI("migrate-uuid") + slog.Debug("args", "c", c) + if err := c.Validate(); err != nil { return err } migrateConfig := LoadMigrationConfigFromCLI() - slog.Debug("args", "migrate-config", migrateConfig) + slog.Debug("args", "migrate-c", migrateConfig) if err := migrateConfig.Validate(); err != nil { return err } authConfig := LoadAuthConfigFromCLI() - slog.Debug("args", "auth-config", authConfig) + slog.Debug("args", "auth-c", authConfig) if err := authConfig.Validate(); err != nil { return err } - slog.Info("Loading state...", "uuid", config.UUID) - item, err := store.LoadState(config.UUID) + slog.Info("Loading state...", "uuid", c.UUID) + item, err := store.LoadState(c.UUID) if err != nil { return errors.WithMessage(err, "unable to load state") } @@ -55,7 +57,7 @@ func MigrateCmdRunE(cmd *cobra.Command, args []string) error { return err } - r := CreateRestClient(cmd.Context(), config.Url, config.Neighborhood) + r := CreateRestClient(cmd.Context(), c.Url, c.Neighborhood) if err := AuthenticateRestClient(r, authConfig.Username, authConfig.Password); err != nil { return err } @@ -95,7 +97,7 @@ func compareItems(item *store.WorkItem, remoteItem *store.WorkItem) error { return nil } -func SetupMigrateCmdFlags(command *cobra.Command) { +func setupStringCmdFlags(command *cobra.Command) { args := []struct { name string key string @@ -125,6 +127,31 @@ func SetupMigrateCmdFlags(command *cobra.Command) { } } +func setupUIntCmdFlags(command *cobra.Command) { + args := []struct { + name string + key string + value uint + usage string + }{ + {"wait-for-tx-timeout", "wait-for-tx-timeout", 15, "Number of seconds spent waiting for the transaction to be included in a block"}, + {"wait-for-block-timeout", "wait-for-block-timeout", 30, "Number of seconds spent waiting for the block to be committed"}, + } + + for _, arg := range args { + command.Flags().Uint(arg.name, arg.value, arg.usage) + if err := viper.BindPFlag(arg.key, command.Flags().Lookup(arg.name)); err != nil { + slog.Error(ErrorBindingFlag, "error", err) + } + } + +} + +func SetupMigrateCmdFlags(command *cobra.Command) { + setupStringCmdFlags(command) + setupUIntCmdFlags(command) +} + func mapToken(symbol string, tokenMap map[string]utils.TokenInfo) (*utils.TokenInfo, error) { if _, ok := tokenMap[symbol]; !ok { return nil, fmt.Errorf("token %s not found in token map", symbol) @@ -134,7 +161,7 @@ func mapToken(symbol string, tokenMap map[string]utils.TokenInfo) (*utils.TokenI } // migrate migrates a work item to the Manifest Ledger. -func migrate(r *resty.Client, item *store.WorkItem, config MigrateConfig) error { +func migrate(r *resty.Client, item *store.WorkItem, config config.MigrateConfig) error { slog.Info("Migrating work item...", "uuid", item.UUID) remoteItem, err := store.GetWorkItem(r, item.UUID) @@ -171,7 +198,7 @@ func migrate(r *resty.Client, item *store.WorkItem, config MigrateConfig) error slog.Debug("Amount before conversion", "amount", txInfo.Arguments.Amount) // Convert the amount to the destination chain precision - // TODO: currentPrecision is hardcoded to 9 for now as all tokens on the MANY network have 9 digits places + // NOTE: currentPrecision is hardcoded to 9 for now as all tokens on the MANY network have 9 digits places amount, err := utils.ConvertPrecision(txInfo.Arguments.Amount, 9, tokenInfo.Precision) if err != nil { return errors.WithMessage(err, "error converting token to destination precision") @@ -247,16 +274,8 @@ func setAsFailed(r *resty.Client, newItem store.WorkItem, errStr *string) error } // sendTokens sends the tokens from the bank account to the user account. -func sendTokens(item *store.WorkItem, config MigrateConfig, denom string, amount *big.Int) (*string, *time.Time, error) { - txResponse, blockTime, err := manifest.Migrate(item, manifest.MigrationConfig{ - ChainID: config.ChainID, - NodeAddress: config.NodeAddress, - KeyringBackend: config.KeyringBackend, - ChainHome: config.ChainHome, - AddressPrefix: config.AddressPrefix, - BankAddress: config.BankAddress, - TokenMap: config.TokenMap, - }, denom, amount) +func sendTokens(item *store.WorkItem, config config.MigrateConfig, denom string, amount *big.Int) (*string, *time.Time, error) { + txResponse, blockTime, err := manifest.Migrate(item, config, denom, amount) if err != nil { return nil, nil, errors.WithMessage(err, "error during migration, operator intervention required") } diff --git a/cmd/verify.go b/cmd/verify.go index 5647f6f..d5101f7 100644 --- a/cmd/verify.go +++ b/cmd/verify.go @@ -16,13 +16,13 @@ var verifyCmd = &cobra.Command{ Use: "verify", Short: "Verify the status of a migration of MFX tokens to the Manifest Ledger", RunE: func(cmd *cobra.Command, args []string) error { - config := LoadConfigFromCLI("verify-uuid") - slog.Debug("args", "config", config) - if err := config.Validate(); err != nil { + c := LoadConfigFromCLI("verify-uuid") + slog.Debug("args", "c", c) + if err := c.Validate(); err != nil { return err } - s, err := store.LoadState(config.UUID) + s, err := store.LoadState(c.UUID) if err != nil { slog.Warn("unable to load local state, continuing", "warning", err) } @@ -32,11 +32,11 @@ var verifyCmd = &cobra.Command{ } // Verify the work item on the remote database - slog.Debug("verifying remote state", "url", config.Url, "uuid", config.UUID) + slog.Debug("verifying remote state", "url", c.Url, "uuid", c.UUID) - r := CreateRestClient(cmd.Context(), config.Url, config.Neighborhood) + r := CreateRestClient(cmd.Context(), c.Url, c.Neighborhood) - item, err := store.GetWorkItem(r, uuid.MustParse(config.UUID)) + item, err := store.GetWorkItem(r, uuid.MustParse(c.UUID)) if err != nil { return errors.WithMessage(err, "unable to get work item") } diff --git a/interchaintest/migrate_on_chain_test.go b/interchaintest/migrate_on_chain_test.go index 2d1de6b..17dea0d 100644 --- a/interchaintest/migrate_on_chain_test.go +++ b/interchaintest/migrate_on_chain_test.go @@ -122,7 +122,6 @@ func TestMigrateOnChain(t *testing.T) { Bank: Amounts{Old: defaultGenesisAmtMinOne, New: math.ZeroInt()}, User: Amounts{Old: math.OneInt(), New: DefaultGenesisAmt}, }}, - // TODO: Add more test cases } for _, tc := range tt { diff --git a/cmd/config.go b/internal/config/config.go similarity index 50% rename from cmd/config.go rename to internal/config/config.go index 3c1d907..30955af 100644 --- a/cmd/config.go +++ b/internal/config/config.go @@ -1,11 +1,10 @@ -package cmd +package config import ( "fmt" "net/url" "github.com/google/uuid" - "github.com/spf13/viper" "github.com/liftedinit/mfx-migrator/internal/utils" ) @@ -45,18 +44,6 @@ func (c Config) Validate() error { return nil } -// LoadConfigFromCLI loads the Config from the CLI flags -// -// `uuidKey` is the name of the viper key that holds the UUID -// This is necessary because the UUID is used in multiple commands -func LoadConfigFromCLI(uuidKey string) Config { - return Config{ - UUID: viper.GetString(uuidKey), - Url: viper.GetString("url"), - Neighborhood: viper.GetUint64("neighborhood"), - } -} - type AuthConfig struct { Username string // The username to authenticate with Password string // The password to authenticate with @@ -78,47 +65,20 @@ func (c AuthConfig) Validate() error { return nil } -func LoadAuthConfigFromCLI() AuthConfig { - return AuthConfig{ - Username: viper.GetString("username"), - Password: viper.GetString("password"), - } -} - type ClaimConfig struct { Force bool // Force re-claiming of a failed work item } -func LoadClaimConfigFromCLI() ClaimConfig { - return ClaimConfig{ - Force: viper.GetBool("force"), - } -} - type MigrateConfig struct { - ChainID string // The destination chain ID - AddressPrefix string // The destination address prefix - NodeAddress string // The destination RPC node address - KeyringBackend string // The destination chain keyring backend to use - BankAddress string // The destination chain address of the bank account to send tokens from - ChainHome string // The root directory of the destination chain configuration - TokenMap map[string]utils.TokenInfo // Map of source token address to destination token info -} - -func LoadMigrationConfigFromCLI() MigrateConfig { - var tokenMap map[string]utils.TokenInfo - if err := viper.UnmarshalKey("token-map", &tokenMap); err != nil { - panic(err) - } - return MigrateConfig{ - ChainID: viper.GetString("chain-id"), - AddressPrefix: viper.GetString("address-prefix"), - NodeAddress: viper.GetString("node-address"), - KeyringBackend: viper.GetString("keyring-backend"), - BankAddress: viper.GetString("bank-address"), - ChainHome: viper.GetString("chain-home"), - TokenMap: tokenMap, - } + ChainID string // The destination chain ID + AddressPrefix string // The destination address prefix + NodeAddress string // The destination RPC node address + KeyringBackend string // The destination chain keyring backend to use + BankAddress string // The destination chain address of the bank account to send tokens from + ChainHome string // The root directory of the destination chain configuration + TokenMap map[string]utils.TokenInfo // Map of source token address to destination token info + WaitTxTimeout uint // Number of seconds spent waiting for the transaction to be included in a block + WaitBlockTimeout uint // Number of seconds spent waiting for the block to be committed } func (c MigrateConfig) Validate() error { @@ -146,5 +106,13 @@ func (c MigrateConfig) Validate() error { return fmt.Errorf("chain home is required") } + if c.WaitTxTimeout == 0 { + return fmt.Errorf("wait for tx timeout > 0 is required") + } + + if c.WaitBlockTimeout == 0 { + return fmt.Errorf("wait for block timeout > 0 is required") + } + return nil } diff --git a/internal/manifest/migrate.go b/internal/manifest/migrate.go index 7ea9562..6353cf4 100644 --- a/internal/manifest/migrate.go +++ b/internal/manifest/migrate.go @@ -28,23 +28,11 @@ import ( stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" "github.com/pkg/errors" - "github.com/liftedinit/mfx-migrator/internal/utils" + "github.com/liftedinit/mfx-migrator/internal/config" "github.com/liftedinit/mfx-migrator/internal/store" ) -// TODO: Refactor & Cleanup - -type MigrationConfig struct { - ChainID string - NodeAddress string - KeyringBackend string - ChainHome string - AddressPrefix string - BankAddress string - TokenMap map[string]utils.TokenInfo -} - const defaultGasLimit uint64 = 200000 // registerInterfaces registers the necessary interfaces and concrete types on the provided InterfaceRegistry. @@ -85,9 +73,9 @@ func newClientContext(chainID, nodeAddress, keyringBackend, chainHomeDir string, } // Migrate migrates the given amount of tokens to the specified address. -func Migrate(item *store.WorkItem, migrateConfig MigrationConfig, denom string, amount *big.Int) (*sdk.TxResponse, *time.Time, error) { - config := sdk.GetConfig() - config.SetBech32PrefixForAccount(migrateConfig.AddressPrefix, migrateConfig.AddressPrefix+"pub") +func Migrate(item *store.WorkItem, migrateConfig config.MigrateConfig, denom string, amount *big.Int) (*sdk.TxResponse, *time.Time, error) { + c := sdk.GetConfig() + c.SetBech32PrefixForAccount(migrateConfig.AddressPrefix, migrateConfig.AddressPrefix+"pub") inBuf := bufio.NewReader(os.Stdin) clientCtx, err := newClientContext(migrateConfig.ChainID, migrateConfig.NodeAddress, migrateConfig.KeyringBackend, migrateConfig.ChainHome, inBuf) @@ -113,7 +101,7 @@ func Migrate(item *store.WorkItem, migrateConfig MigrationConfig, denom string, return nil, nil, errors.WithMessage(err, "failed to prepare transaction") } - res, blockTime, err := signAndBroadcast(clientCtx, txBuilder, migrateConfig.BankAddress, info) + res, blockTime, err := signAndBroadcast(clientCtx, txBuilder, migrateConfig.BankAddress, info, migrateConfig.WaitTxTimeout, migrateConfig.WaitBlockTimeout) if err != nil { return nil, nil, errors.WithMessage(err, "failed to sign and broadcast transaction") } @@ -155,7 +143,7 @@ func prepareTx(ctx client.Context, msg sdk.Msg, memo, denom string) (client.TxBu } // signAndBroadcast signs and broadcasts the transaction, returning the transaction response and block time. -func signAndBroadcast(ctx client.Context, txBuilder client.TxBuilder, bankAccount string, info *keyring.Record) (*sdk.TxResponse, *time.Time, error) { +func signAndBroadcast(ctx client.Context, txBuilder client.TxBuilder, bankAccount string, info *keyring.Record, waitForTxTimeout, blockTimeout uint) (*sdk.TxResponse, *time.Time, error) { txFactory := tx.Factory{}. WithChainID(ctx.ChainID). WithKeybase(ctx.Keyring). @@ -204,7 +192,7 @@ func signAndBroadcast(ctx client.Context, txBuilder client.TxBuilder, bankAccoun slog.Info("Transaction broadcasted", "hash", res.TxHash) // Wait for the transaction to be included in a block - txResult, err := waitForTx(ctx.Client, res.TxHash) + txResult, err := waitForTx(ctx.Client, res.TxHash, waitForTxTimeout, blockTimeout) if err != nil { return nil, nil, errors.WithMessage(err, "failed to wait for transaction") } @@ -227,15 +215,14 @@ func signAndBroadcast(ctx client.Context, txBuilder client.TxBuilder, bankAccoun } // waitForTx waits for a transaction to be included in a block. -func waitForTx(rClient client.CometRPC, hash string) (*coretypes.ResultTx, error) { +func waitForTx(rClient client.CometRPC, hash string, txTimeout, blockTimeout uint) (*coretypes.ResultTx, error) { bHash, err := hex.DecodeString(hash) if err != nil { return nil, errors.WithMessage(err, "failed to decode hash") } // Create a context that will be cancelled after the specified timeout - // TODO: Configure timeout - ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), time.Duration(txTimeout)*time.Second) defer cancel() for { @@ -247,7 +234,7 @@ func waitForTx(rClient client.CometRPC, hash string) (*coretypes.ResultTx, error r, tErr := rClient.Tx(context.Background(), bHash, false) if tErr != nil { if strings.Contains(tErr.Error(), "not found") { - if cErr := waitForNextBlock(rClient); cErr != nil { + if cErr := waitForNextBlock(rClient, blockTimeout); cErr != nil { return nil, errors.WithMessage(cErr, "failed to wait for next block") } continue @@ -267,7 +254,7 @@ func getLatestBlockHeight(client client.CometRPC) (int64, error) { return status.SyncInfo.LatestBlockHeight, nil } -func waitForBlockHeight(client client.CometRPC, height int64) error { +func waitForBlockHeight(client client.CometRPC, height int64, timeout uint) error { ticker := time.NewTicker(time.Second) defer ticker.Stop() @@ -281,17 +268,16 @@ func waitForBlockHeight(client client.CometRPC, height int64) error { if latestHeight >= height { return nil } - case <-time.After(30 * time.Second): - // TODO: Configure timeout + case <-time.After(time.Duration(timeout) * time.Second): return fmt.Errorf("timeout exceeded waiting for block") } } } -func waitForNextBlock(client client.CometRPC) error { +func waitForNextBlock(client client.CometRPC, timeout uint) error { latestHeight, err := getLatestBlockHeight(client) if err != nil { return errors.WithMessage(err, "failed to get latest block height") } - return waitForBlockHeight(client, latestHeight+1) + return waitForBlockHeight(client, latestHeight+1, timeout) } diff --git a/internal/store/claim_test.go b/internal/store/claim_test.go index 425c4b2..21090b2 100644 --- a/internal/store/claim_test.go +++ b/internal/store/claim_test.go @@ -30,7 +30,6 @@ func TestStore_Claim(t *testing.T) { rClient := resty.New().SetBaseURL(testUrl.String()).SetPathParam("neighborhood", testutils.Neighborhood) httpmock.ActivateNonDefault(rClient.GetClient()) - // TODO: Test force claim of a failed item clears the previous error var tests = []testCase{ // Successfully claim a work item from the queue {"success_queue", []testutils.HttpResponder{ diff --git a/testutils/httpresponder.go b/testutils/httpresponder.go index 168c74c..50f5053 100644 --- a/testutils/httpresponder.go +++ b/testutils/httpresponder.go @@ -13,6 +13,8 @@ import ( "github.com/liftedinit/mfx-migrator/internal/store" ) +// TODO: Randomize parameters. Fuzzy testing. + const ( Uuid = "5aa19d2a-4bdf-4687-a850-1804756b3f1f" ManyFrom = "maffbahksdwaqeenayy2gxke32hgb7aq4ao4wt745lsfs6wijp" @@ -31,7 +33,6 @@ type HttpResponder struct { var AuthResponder, _ = httpmock.NewJsonResponder(http.StatusOK, map[string]string{"access_token": "ya29.Gl0UBZ3"}) -// TODO: Random func MustNewTransactionResponseResponder(amount string) httpmock.Responder { response := many.TxInfo{Arguments: many.Arguments{ From: ManyFrom, @@ -47,7 +48,6 @@ func MustNewTransactionResponseResponder(amount string) httpmock.Responder { return transactionResponseResponder } -// TODO: Random func MustAllMigrationsGetResponder(nb uint, status store.WorkItemStatus) httpmock.Responder { if nb > 1 { panic("nb must be <= 1") @@ -86,7 +86,6 @@ func MustAllMigrationsGetResponder(nb uint, status store.WorkItemStatus) httpmoc var NotFoundResponder, _ = httpmock.NewJsonResponder(http.StatusNotFound, nil) var GarbageResponder, _ = httpmock.NewJsonResponder(http.StatusOK, "{\"foo\": \"bar\"") -// TODO: Random func MustMigrationGetResponder(status store.WorkItemStatus) httpmock.Responder { var failedErr *string sErr := "some error"