Review

modules/apps/100-atomic-swap/client/cli/query.go

@@ -40,7 +40,7 @@ func GetCmdParams() *cobra.Command {
 	return cmd
 }
 
-// GetCmdQueryEscrowAddress returns the command handler for ibc-swap parameter querying.
+// GetCmdQueryEscrowAddress returns the escrow address for a particular port and channel id.
 func GetCmdQueryEscrowAddress() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:     "escrow-address",
@@ -65,7 +65,7 @@ func GetCmdQueryEscrowAddress() *cobra.Command {
 	return cmd
 }
 
-// GetCmdQueryEscrowAddress returns the command handler for ibc-swap parameter querying.
+// GetCmdOrderList returns all the orders
 func GetCmdOrderList() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:     "orders",

modules/apps/100-atomic-swap/client/cli/reveiw.md

@@ -0,0 +1,5 @@
+# Review
+
+- client/cli: `Add tx_test.go` Add test cases to verify if parsing is done correctly for all the parameters. Should throw errors for invalid values.
+- Add `example` to all the commands
+- Long description is same for every command

modules/apps/100-atomic-swap/client/cli/tx.go

@@ -174,6 +174,7 @@ corresponding to the counterparty channel. Any timeout set to 0 is disabled.`),
 				return err
 			}
 
+			// TODO: Remove comments if not needed
 			//absoluteTimeouts, err := cmd.Flags().GetBool(flagAbsoluteTimeouts)
 			//if err != nil {
 			//	return err

modules/apps/100-atomic-swap/keeper/msg_server.go

@@ -232,7 +232,11 @@ func (k Keeper) OnReceivedCancel(ctx sdk.Context, packet channeltypes.Packet, ms
 }
 
 func createOrder(ctx sdk.Context, msg *types.MakeSwapMsg, channelKeeper types.ChannelKeeper) types.Order {
+	// What if channel is not found ?
 	channel, _ := channelKeeper.GetChannel(ctx, msg.SourcePort, msg.SourceChannel)
+	// What if sequence is not found ?
+	// Will this sequence always be different ? Order ID duplication depends on it.
+	// Should we add a check for duplicate order id ?
 	sequence, _ := channelKeeper.GetNextSequenceSend(ctx, msg.SourcePort, msg.SourceChannel)
 	path := orderPath(msg.SourcePort, msg.SourceChannel, channel.Counterparty.PortId, channel.Counterparty.ChannelId, sequence)
 	return types.Order{
@@ -250,11 +254,13 @@ func orderPath(sourcePort, sourceChannel, destPort, destChannel string, sequence
 
 func generateOrderId(orderPath string, msg *types.MakeSwapMsg) string {
 	prefix := []byte(orderPath)
+	// error handling is not done, what if marshaljson fails ?
 	bytes, _ := types.ModuleCdc.MarshalJSON(msg)
 	hash := sha256.Sum256(append(prefix, bytes...))
 	return hex.EncodeToString(hash[:])
 }
 
+// Optional: Add tests for creating order path and extracting channel, port
 func extractSourceChannelForTakerMsg(path string) string {
 	parts := strings.Split(path, "/")
 	return parts[5]

modules/apps/100-atomic-swap/keeper/msg_server_cancel_swap.go

@@ -27,6 +27,8 @@ func (k Keeper) CancelSwap(goCtx context.Context, msg *types.CancelSwapMsg) (*ty
 	}
 
 	// Make sure the sender is the maker of the order.
+	// TODO: New Message logic for cancel swap is incomplete. (using command line)
+	// We should verify/make sure that msg.MakerAddress and sender of TX is same. Otherwise this should fail
 	if order.Maker.MakerAddress != msg.MakerAddress {
 		return &types.MsgCancelSwapResponse{}, fmt.Errorf("sender is not the maker of the order")
 	}

modules/apps/100-atomic-swap/keeper/msg_server_take_swap.go

@@ -83,5 +83,5 @@ func (k Keeper) TakeSwap(goCtx context.Context, msg *types.TakeSwapMsg) (*types.
 
 	ctx.EventManager().EmitTypedEvents(msg)
 
-	return &types.MsgTakeSwapResponse{}, nil
+	return &types.MsgTakeSwapResponse{OrderId: msg.OrderId}, nil
 }

modules/apps/100-atomic-swap/keeper/msg_server_test.go

@@ -74,3 +74,5 @@ func (suite *KeeperTestSuite) TestMsgSwap() {
 		}
 	}
 }
+
+// Add tests for take swap message and cancel swap message