Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
refactor: remove SendTransfer, require IBC transfers to be initiated with MsgTransfer #2446
refactor: remove SendTransfer, require IBC transfers to be initiated with MsgTransfer #2446
Changes from 2 commits
4db8313
d2da1c9
e87087b
7549663
05098fb
c7bb641
1f03958
a11a447
a3d80c7
1b41b9b
9351467
dabfc5c
9c4d411
a878376
cca0b61
9e03e65
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like using suite.Run because it adds the test case name to the output if it fails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason to move these checks to the message server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the message server was added after send enabled, which is why it ended up within "SendTransfer"
The reasoning for why I moved it is because I view it as auxiliary to the transfer logic. It isn't in the IBC specification for ICS20
I visualize the code like this:
I think this should be true for most of our handlers:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I think we should avoid overloading functions. Basic enablement checks make sense to go directly in the entrypoint (msg_server.go), while functions nested deeper in the handler can focus on ICS logic entirely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree++
Definitely a good way to reason about things!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should also add here checks for changes that we expect to have happened in case of success (e.g. the expected amount is escrowed in the escrow account)? We check for this already in the e2e tests but maybe it's good to add here for redundancy?
Same comment maybe for the failure cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should. But maybe we should do in a followup issue so as not to increase the scope of this one too much?