Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review #39

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Review #39

wants to merge 2 commits into from

Conversation

amityadav0
Copy link

No description provided.

@amityadav0 amityadav0 requested a review from liangping June 1, 2023 09:46
Comment on lines +235 to +239
// 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 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems use a random string is much reliable.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

msg.MakeAddress should be the sender in the design.

@soring323 soring323 force-pushed the dev branch 3 times, most recently from ef91bae to 7134dc7 Compare July 5, 2023 20:41
@soring323 soring323 force-pushed the dev branch 17 times, most recently from b24e096 to 34e1dee Compare July 26, 2023 08:46
@soring323 soring323 force-pushed the dev branch 3 times, most recently from 8757dd1 to c09cafd Compare August 9, 2023 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants