cleanup todos, either fix or add issue numbers (#166)

protocol/backend/backend.go

@@ -71,6 +71,7 @@ type Backend interface {
 	SetGasPrice(uint64)
 	SetEthAddress(ethcommon.Address)
 	SetXMRDepositAddress(mcrypto.Address, types.Hash)
+	ClearXMRDepositAddress(types.Hash)
 	SetBaseXMRDepositAddress(mcrypto.Address)
 	SetContract(*swapfactory.SwapFactory)
 	SetContractAddress(ethcommon.Address)
@@ -357,12 +358,19 @@ func (b *backend) SetBaseXMRDepositAddress(addr mcrypto.Address) {
 func (b *backend) SetXMRDepositAddress(addr mcrypto.Address, id types.Hash) {
 	b.Lock()
 	defer b.Unlock()
-	// TODO: clear this out when swap is done, memory leak!!!
 	b.xmrDepositAddrs[id] = addr
 }
 
-// TODO: these are kinda sus, maybe remove them? forces everyone to use
-// the same contract though
+func (b *backend) ClearXMRDepositAddress(id types.Hash) {
+	b.Lock()
+	defer b.Unlock()
+	delete(b.xmrDepositAddrs, id)
+}
+
+// NOTE: this is called when a swap is initiated and the XMR-taker specifies the contract
+// address they will be using.
+// the contract bytecode is validated in the calling code, but this should never be called
+// for unvalidated contracts.
 func (b *backend) SetContract(contract *swapfactory.SwapFactory) {
 	b.contract = contract
 	b.Sender.SetContract(contract)

protocol/swap/manager.go

@@ -21,21 +21,33 @@ type Info struct {
 	statusCh       <-chan types.Status
 }
 
+// NewInfo ...
+func NewInfo(id types.Hash, provides types.ProvidesCoin, providedAmount, receivedAmount float64,
+	exchangeRate types.ExchangeRate, status Status, statusCh <-chan types.Status) *Info {
+	info := &Info{
+		id:             id,
+		provides:       provides,
+		providedAmount: providedAmount,
+		receivedAmount: receivedAmount,
+		exchangeRate:   exchangeRate,
+		status:         status,
+		statusCh:       statusCh,
+	}
+	return info
+}
+
+// NewEmptyInfo returns an empty *Info
+func NewEmptyInfo() *Info {
+	return &Info{}
+}
+
 // ID returns the swap ID.
 func (i *Info) ID() types.Hash {
-	if i == nil {
-		return types.Hash{} // TODO: does this ever happen??
-	}
-
 	return i.id
 }
 
 // Provides returns the coin that was provided for this swap.
 func (i *Info) Provides() types.ProvidesCoin {
-	if i == nil {
-		return ""
-	}
-
 	return i.provides
 }
 
@@ -56,10 +68,6 @@ func (i *Info) ExchangeRate() types.ExchangeRate {
 
 // Status returns the swap's status.
 func (i *Info) Status() Status {
-	if i == nil {
-		return 0
-	}
-
 	return i.status
 }
 
@@ -70,28 +78,9 @@ func (i *Info) StatusCh() <-chan types.Status {
 
 // SetStatus ...
 func (i *Info) SetStatus(s Status) {
-	if i == nil {
-		return
-	}
-
 	i.status = s
 }
 
-// NewInfo ...
-func NewInfo(id types.Hash, provides types.ProvidesCoin, providedAmount, receivedAmount float64,
-	exchangeRate types.ExchangeRate, status Status, statusCh <-chan types.Status) *Info {
-	info := &Info{
-		id:             id,
-		provides:       provides,
-		providedAmount: providedAmount,
-		receivedAmount: receivedAmount,
-		exchangeRate:   exchangeRate,
-		status:         status,
-		statusCh:       statusCh,
-	}
-	return info
-}
-
 // Manager tracks current and past swaps.
 type Manager interface {
 	AddSwap(info *Info) error

protocol/xmrmaker/message_handler.go

@@ -103,9 +103,8 @@ func (s *swapState) setNextExpectedMessage(msg net.Message) {
 	}
 
 	s.nextExpectedMessage = msg
-	// TODO: check stage is not unknown (ie. swap completed)
 	stage := pcommon.GetStatus(msg.Type())
-	if s.statusCh != nil {
+	if s.statusCh != nil && stage != types.UnknownStatus {
 		s.statusCh <- stage
 	}
 }
@@ -171,7 +170,7 @@ func (s *swapState) handleNotifyETHLocked(msg *message.NotifyETHLocked) (net.Mes
 		return nil, err
 	}
 
-	// TODO: check these (in checkContract)
+	// TODO: check these (in checkContract) (#161)
 	s.setTimeouts(msg.ContractSwap.Timeout0, msg.ContractSwap.Timeout1)
 
 	addrAB, err := s.lockFunds(common.MoneroToPiconero(s.info.ProvidedAmount()))
@@ -211,7 +210,8 @@ func (s *swapState) runT0ExpirationHandler() {
 		return
 	case err := <-waitCh:
 		if err != nil {
-			// TODO: Do we propagate this error? If we retry, the logic should probably be inside WaitForTimestamp.
+			// TODO: Do we propagate this error? If we retry, the logic should probably be inside
+			// WaitForTimestamp. (#162)
 			log.Errorf("Failure waiting for T0 timeout: err=%s", err)
 			return
 		}
@@ -232,7 +232,7 @@ func (s *swapState) handleT0Expired() {
 	txHash, err := s.claimFunds()
 	if err != nil {
 		log.Errorf("failed to claim: err=%s", err)
-		// TODO: retry claim, depending on error
+		// TODO: retry claim, depending on error (#162)
 		if err = s.exit(); err != nil {
 			log.Errorf("exit failed: err=%s", err)
 		}

protocol/xmrmaker/mocks_test.go

@@ -108,6 +108,18 @@ func (mr *MockBackendMockRecorder) Claim(arg0, arg1, arg2 interface{}) *gomock.C
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Claim", reflect.TypeOf((*MockBackend)(nil).Claim), arg0, arg1, arg2)
 }
 
+// ClearXMRDepositAddress mocks base method.
+func (m *MockBackend) ClearXMRDepositAddress(arg0 types.Hash) {
+	m.ctrl.T.Helper()
+	m.ctrl.Call(m, "ClearXMRDepositAddress", arg0)
+}
+
+// ClearXMRDepositAddress indicates an expected call of ClearXMRDepositAddress.
+func (mr *MockBackendMockRecorder) ClearXMRDepositAddress(arg0 interface{}) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ClearXMRDepositAddress", reflect.TypeOf((*MockBackend)(nil).ClearXMRDepositAddress), arg0)
+}
+
 // CloseWallet mocks base method.
 func (m *MockBackend) CloseWallet() error {
 	m.ctrl.T.Helper()

protocol/xmrmaker/offers/offer_manager.go

@@ -7,6 +7,8 @@ import (
 	pcommon "github.com/athanorlabs/atomic-swap/protocol"
 )
 
+const statusChSize = 6 // the max number of stages a swap can potentially go through
+
 // Manager synchronises access to the offers map.
 type Manager struct {
 	mu       sync.Mutex // synchronises access to the offers map
@@ -53,7 +55,7 @@ func (m *Manager) AddOffer(o *types.Offer) *types.OfferExtra {
 	}
 
 	extra := &types.OfferExtra{
-		StatusCh: make(chan types.Status, 7), // TODO: Constant for this? How was 7 picked?
+		StatusCh: make(chan types.Status, statusChSize),
 		InfoFile: pcommon.GetSwapInfoFilepath(m.basePath),
 	}
 

protocol/xmrmaker/swap_state.go

@@ -32,11 +32,7 @@ import (
 
 const revertSwapCompleted = "swap is already completed"
 
-var (
-	// this is from the autogenerated swap.go
-	// TODO: generate this ourselves instead of hard-coding
-	refundedTopic = ethcommon.HexToHash("0x007c875846b687732a7579c19bb1dade66cd14e9f4f809565e2b2b5e76c72b4f")
-)
+var refundedTopic = common.GetTopic(common.RefundedEventSignature)
 
 type swapState struct {
 	backend.Backend
@@ -225,13 +221,13 @@ func (s *swapState) exit() error {
 			log.Errorf("failed to check for refund: err=%s", err)
 
 			// TODO: depending on the error, we should either retry to refund or try to claim.
-			// we should wait for both events in the contract and proceed accordingly.
+			// we should wait for both events in the contract and proceed accordingly. (#162)
 			//
 			// we already locked our funds - need to wait until we can claim
 			// the funds (ie. wait until after t0)
 			txHash, err := s.tryClaim()
 			if err != nil {
-				// TODO: this shouldn't happen, as it means we had a race condition somewhere
+				// note: this shouldn't happen, as it means we had a race condition somewhere
 				if strings.Contains(err.Error(), revertSwapCompleted) && !s.info.Status().IsOngoing() {
 					return nil
 				}
@@ -243,7 +239,7 @@ func (s *swapState) exit() error {
 				return nil
 			}
 
-			// TODO: keep retrying until success
+			// TODO: keep retrying until success (#162)
 			return err
 		}
 
@@ -282,7 +278,6 @@ func (s *swapState) reclaimMonero(skA *mcrypto.PrivateSpendKey) (mcrypto.Address
 		return "", err
 	}
 
-	// TODO: check balance
 	s.LockClient()
 	defer s.UnlockClient()
 	return monero.CreateWallet("xmrmaker-swap-wallet", s.Env(), s, kpAB)
@@ -365,7 +360,7 @@ func (s *swapState) tryClaim() (ethcommon.Hash, error) {
 	}
 
 	if ts.Before(s.t0) && stage != swapfactory.StageReady {
-		// TODO: t0 could be 24 hours from now. Don't we want to poll the stage periodically?
+		// TODO: t0 could be 24 hours from now. Don't we want to poll the stage periodically? (#163)
 		// we need to wait until t0 to claim
 		log.Infof("waiting until time %s to claim, time now=%s", s.t0, time.Now())
 		err = s.WaitForTimestamp(s.ctx, s.t0)
@@ -473,7 +468,7 @@ func (s *swapState) checkContract(txHash ethcommon.Hash) error {
 		return fmt.Errorf("contract refund key is not expected: got 0x%x, expected 0x%x", event.RefundKey, skTheirs)
 	}
 
-	// TODO: check timeouts
+	// TODO: check timeouts (#161)
 
 	// check value of created swap
 	value := s.contractSwap.Value
@@ -488,7 +483,6 @@ func (s *swapState) checkContract(txHash ethcommon.Hash) error {
 // lockFunds locks XMRMaker's funds in the monero account specified by public key
 // (S_a + S_b), viewable with (V_a + V_b)
 // It accepts the amount to lock as the input
-// TODO: units
 func (s *swapState) lockFunds(amount common.MoneroAmount) (mcrypto.Address, error) {
 	kp := mcrypto.SumSpendAndViewKeys(s.xmrtakerPublicKeys, s.pubkeys)
 	log.Infof("going to lock XMR funds, amount(piconero)=%d", amount)

protocol/xmrtaker/instance.go

@@ -55,15 +55,21 @@ func NewInstance(cfg *Config) (*Instance, error) {
 		err     error
 	)
 
+	// if this is set, it transfers all xmr received during swaps back to the given wallet.
 	if cfg.TransferBack {
 		address, err = getAddress(cfg.Backend, cfg.MoneroWalletFile, cfg.MoneroWalletPassword)
 		if err != nil {
 			return nil, err
 		}
 		cfg.Backend.SetBaseXMRDepositAddress(address)
+	} else {
+		// check that XMRTaker's monero-wallet-cli endpoint has wallet-dir configured
+		err = checkWalletDir(cfg.Backend)
+		if err != nil {
+			return nil, err
+		}
 	}
 
-	// TODO: check that XMRTaker's monero-wallet-cli endpoint has wallet-dir configured
 	return &Instance{
 		backend:        cfg.Backend,
 		basepath:       cfg.Basepath,
@@ -73,6 +79,16 @@ func NewInstance(cfg *Config) (*Instance, error) {
 	}, nil
 }
 
+func checkWalletDir(walletClient monero.WalletClient) error {
+	// don't need to check error here, since if there's no wallet open that's fine
+	_ = walletClient.CloseWallet()
+	err := walletClient.CreateWallet(swapDepositWallet, "")
+	if err != nil {
+		return err
+	}
+	return walletClient.CloseWallet()
+}
+
 func getAddress(walletClient monero.WalletClient, file, password string) (mcrypto.Address, error) {
 	// open XMR wallet, if it exists
 	if file != "" {
@@ -80,7 +96,6 @@ func getAddress(walletClient monero.WalletClient, file, password string) (mcrypt
 			return "", err
 		}
 	} else {
-		// TODO: prompt user for wallet or error if not in dev mode
 		log.Info("monero wallet file not set; creating wallet swap-deposit-wallet")
 		err := walletClient.CreateWallet(swapDepositWallet, "")
 		if err != nil {

protocol/xmrtaker/message_handler.go

@@ -80,9 +80,8 @@ func (s *swapState) setNextExpectedMessage(msg net.Message) {
 
 	s.nextExpectedMessage = msg
 
-	// TODO: check stage is not unknown (ie. swap completed)
 	stage := pcommon.GetStatus(msg.Type())
-	if s.statusCh != nil {
+	if s.statusCh != nil && stage != types.UnknownStatus {
 		s.statusCh <- stage
 	}
 }
@@ -240,7 +239,7 @@ func (s *swapState) handleNotifyXMRLock(msg *message.NotifyXMRLock) (net.Message
 	if s.Env() != common.Development {
 		log.Infof("waiting for new blocks...")
 		// wait for 2 new blocks, otherwise balance might be 0
-		// TODO: check transaction hash
+		// TODO: check transaction hash (#164)
 		height, err := monero.WaitForBlocks(s.Backend, 2)
 		if err != nil {
 			return nil, err
@@ -283,12 +282,18 @@ func (s *swapState) handleNotifyXMRLock(msg *message.NotifyXMRLock) (net.Message
 
 	log.Debugf("checking locked wallet, address=%s balance=%v", kp.Address(s.Env()), balance.Balance)
 
-	// TODO: also check that the balance isn't unlocked only after an unreasonable amount of blocks
 	if balance.Balance < uint64(s.receivedAmountInPiconero()) {
 		return nil, fmt.Errorf("locked XMR amount is less than expected: got %v, expected %v",
 			balance.Balance, float64(s.receivedAmountInPiconero()))
 	}
 
+	// also check that the balance isn't unlocked only after an unreasonable amount of blocks
+	// somewhat arbitrarily chosen as 10x the default unlock time
+	// maybe make this configurable?
+	if balance.BlocksToUnlock > 100 {
+		return nil, fmt.Errorf("locked XMR unlocks too far into the future: got %d blocks", balance.BlocksToUnlock)
+	}
+
 	if err := s.CloseWallet(); err != nil {
 		return nil, fmt.Errorf("failed to close wallet: %w", err)
 	}
@@ -328,7 +333,8 @@ func (s *swapState) runT1ExpirationHandler() {
 		return
 	case err := <-waitCh:
 		if err != nil {
-			// TODO: Do we propagate this error? If we retry, the logic should probably be inside WaitForTimestamp.
+			// TODO: Do we propagate this error? If we retry, the logic should probably be inside
+			// WaitForTimestamp. (#162)
 			log.Errorf("Failure waiting for T1 timeout: err=%s", err)
 			return
 		}
@@ -352,7 +358,6 @@ func (s *swapState) handleT1Expired() {
 	}
 
 	log.Infof("got our ETH back: tx hash=%s", txhash)
-	s.clearNextExpectedMessage(types.CompletedRefund) // TODO: duplicate?
 
 	// send NotifyRefund msg
 	if err = s.SendSwapMessage(&message.NotifyRefund{

protocol/xmrtaker/recovery.go

@@ -9,15 +9,16 @@ import (
 	ethcommon "github.com/ethereum/go-ethereum/common"
 	ethtypes "github.com/ethereum/go-ethereum/core/types"
 
+	"github.com/athanorlabs/atomic-swap/common"
 	mcrypto "github.com/athanorlabs/atomic-swap/crypto/monero"
 	"github.com/athanorlabs/atomic-swap/dleq"
 	pcommon "github.com/athanorlabs/atomic-swap/protocol"
 	"github.com/athanorlabs/atomic-swap/protocol/backend"
+	pswap "github.com/athanorlabs/atomic-swap/protocol/swap"
 	"github.com/athanorlabs/atomic-swap/swapfactory"
 )
 
-// TODO: don't hard-code this
-var claimedTopic = ethcommon.HexToHash("0x38d6042dbdae8e73a7f6afbabd3fbe0873f9f5ed3cd71294591c3908c2e65fee")
+var claimedTopic = common.GetTopic(common.ClaimedEventSignature)
 
 type recoveryState struct {
 	ss *swapState
@@ -49,6 +50,7 @@ func NewRecoveryState(b backend.Backend, basePath string, secret *mcrypto.Privat
 		contractSwap:   contractSwap,
 		infoFile:       pcommon.GetSwapRecoveryFilepath(basePath),
 		claimedCh:      make(chan struct{}),
+		info:           pswap.NewEmptyInfo(),
 	}
 
 	rs := &recoveryState{

protocol/xmrtaker/swap_state.go

@@ -348,6 +348,7 @@ func (s *swapState) tryRefund() (ethcommon.Hash, error) {
 		if err == nil || !strings.Contains(err.Error(), revertUnableToRefund) {
 			return txHash, err
 		}
+
 		// There is a small, but non-zero chance that our transaction gets placed in a block that is after T0
 		// even though the current block is before T0. In this case, the transaction will be reverted, the
 		// gas fee is lost, but we can wait until T1 and try again.
@@ -478,7 +479,7 @@ func (s *swapState) ready() error {
 	if stage != swapfactory.StagePending {
 		return fmt.Errorf("can not set contract to ready when swap stage is %s", swapfactory.StageToString(stage))
 	}
-	_, _, err = s.SetReady(s.ID(), s.contractSwap)
+	_, _, err = s.SetReady(types.EmptyHash, s.contractSwap)
 	if err != nil {
 		if strings.Contains(err.Error(), revertSwapCompleted) && !s.info.Status().IsOngoing() {
 			return nil