From 8f1ee1cf3748bdf6cbd28fd9bb06c09861be61f5 Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Thu, 1 Jun 2023 14:34:51 +0530 Subject: [PATCH 1/2] first review --- modules/apps/100-atomic-swap/client/cli/query.go | 4 ++-- modules/apps/100-atomic-swap/client/cli/reveiw.md | 5 +++++ modules/apps/100-atomic-swap/keeper/msg_server.go | 6 ++++++ .../apps/100-atomic-swap/keeper/msg_server_cancel_swap.go | 2 ++ modules/apps/100-atomic-swap/keeper/msg_server_take_swap.go | 2 +- modules/apps/100-atomic-swap/keeper/msg_server_test.go | 2 ++ 6 files changed, 18 insertions(+), 3 deletions(-) create mode 100644 modules/apps/100-atomic-swap/client/cli/reveiw.md diff --git a/modules/apps/100-atomic-swap/client/cli/query.go b/modules/apps/100-atomic-swap/client/cli/query.go index d27752f8..c06255ae 100644 --- a/modules/apps/100-atomic-swap/client/cli/query.go +++ b/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", diff --git a/modules/apps/100-atomic-swap/client/cli/reveiw.md b/modules/apps/100-atomic-swap/client/cli/reveiw.md new file mode 100644 index 00000000..64802d4f --- /dev/null +++ b/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 \ No newline at end of file diff --git a/modules/apps/100-atomic-swap/keeper/msg_server.go b/modules/apps/100-atomic-swap/keeper/msg_server.go index 4790d680..9d7f7d43 100644 --- a/modules/apps/100-atomic-swap/keeper/msg_server.go +++ b/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] diff --git a/modules/apps/100-atomic-swap/keeper/msg_server_cancel_swap.go b/modules/apps/100-atomic-swap/keeper/msg_server_cancel_swap.go index 6df6fe03..bd51d684 100644 --- a/modules/apps/100-atomic-swap/keeper/msg_server_cancel_swap.go +++ b/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. + // 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") } diff --git a/modules/apps/100-atomic-swap/keeper/msg_server_take_swap.go b/modules/apps/100-atomic-swap/keeper/msg_server_take_swap.go index 2cd4db84..e06c2de3 100644 --- a/modules/apps/100-atomic-swap/keeper/msg_server_take_swap.go +++ b/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 } diff --git a/modules/apps/100-atomic-swap/keeper/msg_server_test.go b/modules/apps/100-atomic-swap/keeper/msg_server_test.go index 90bfc23e..b88a8a93 100644 --- a/modules/apps/100-atomic-swap/keeper/msg_server_test.go +++ b/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 From bf794a8aea1d689d9d4822d535729e9b3444a794 Mon Sep 17 00:00:00 2001 From: Amit Yadav Date: Thu, 1 Jun 2023 14:41:25 +0530 Subject: [PATCH 2/2] comment --- modules/apps/100-atomic-swap/client/cli/tx.go | 1 + modules/apps/100-atomic-swap/keeper/msg_server_cancel_swap.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/apps/100-atomic-swap/client/cli/tx.go b/modules/apps/100-atomic-swap/client/cli/tx.go index 2df481f6..cb800ab2 100644 --- a/modules/apps/100-atomic-swap/client/cli/tx.go +++ b/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 diff --git a/modules/apps/100-atomic-swap/keeper/msg_server_cancel_swap.go b/modules/apps/100-atomic-swap/keeper/msg_server_cancel_swap.go index bd51d684..5ee529d3 100644 --- a/modules/apps/100-atomic-swap/keeper/msg_server_cancel_swap.go +++ b/modules/apps/100-atomic-swap/keeper/msg_server_cancel_swap.go @@ -27,7 +27,7 @@ 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. + // 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")