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

refactor: remove SendTransfer, require IBC transfers to be initiated with MsgTransfer #2446

Merged
merged 16 commits into from
Oct 11, 2022

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Sep 29, 2022

Description

closes: #1918


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

Remove SendTransfer function. MsgTransfer should be used which will call sendTransfer
Tests refactored to utilize MsgTransfer. Simplified test logic.
@colin-axner colin-axner changed the title refactor: remove SendTransfer public function in favor of MsgTransfer refactor: remove SendTransfer, require IBC transfers to be initiated with MsgTransfer Sep 29, 2022
@@ -43,29 +50,31 @@ func (suite *KeeperTestSuite) TestMsgTransfer() {
}

for _, tc := range testCases {
suite.SetupTest()
suite.Run(tc.name, func() {
Copy link
Contributor Author

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

modules/apps/transfer/keeper/relay_test.go Show resolved Hide resolved
modules/apps/transfer/keeper/relay_test.go Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2022

Codecov Report

Merging #2446 (cca0b61) into main (4bd05c6) will increase coverage by 0.04%.
The diff coverage is 66.66%.

❗ Current head cca0b61 differs from pull request most recent head 9e03e65. Consider uploading reports for the commit 9e03e65 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2446      +/-   ##
==========================================
+ Coverage   78.70%   78.74%   +0.04%     
==========================================
  Files         178      178              
  Lines       12311    12298      -13     
==========================================
- Hits         9689     9684       -5     
+ Misses       2195     2190       -5     
+ Partials      427      424       -3     
Impacted Files Coverage Δ
modules/apps/29-fee/types/msgs.go 90.43% <ø> (-0.33%) ⬇️
modules/core/02-client/types/msgs.go 75.73% <ø> (ø)
.../27-interchain-accounts/controller/types/params.go 47.82% <33.33%> (ø)
...s/apps/27-interchain-accounts/host/types/params.go 42.10% <33.33%> (ø)
modules/apps/transfer/types/params.go 48.00% <40.00%> (ø)
modules/apps/transfer/ibc_module.go 64.96% <66.66%> (+0.03%) ⬆️
modules/apps/transfer/keeper/msg_server.go 100.00% <100.00%> (ø)
modules/apps/transfer/keeper/relay.go 90.63% <100.00%> (+2.56%) ⬆️
modules/apps/transfer/types/msgs.go 95.12% <100.00%> (-0.12%) ⬇️
modules/apps/transfer/types/packet.go 92.00% <100.00%> (+0.33%) ⬆️
... and 1 more

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Nice work on the tests! 👏

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -83,14 +58,6 @@ func (k Keeper) sendTransfer(
timeoutHeight clienttypes.Height,
timeoutTimestamp uint64,
) error {
if !k.GetSendEnabled(ctx) {
Copy link
Contributor

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?

Copy link
Contributor Author

@colin-axner colin-axner Oct 3, 2022

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:

// entry point
MsgTransfer

// basic checks/auxiliary logic
IsSendEnabeld?
IsSenderBlocked?

// perform ics20 logic
sendTransfer

I think this should be true for most of our handlers:

// single entry point, Msg...

// basic checks in msg_server.go
isEnabled?
object.ValidateBasic()

// perform logic (typically relay.go)

Copy link
Contributor Author

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

Copy link
Member

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!

modules/apps/transfer/keeper/msg_server_test.go Outdated Show resolved Hide resolved
if tc.expPass {
suite.Require().NoError(err)
suite.Require().NotNil(res)
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Super clean refactor. Nice work!

@@ -83,14 +58,6 @@ func (k Keeper) sendTransfer(
timeoutHeight clienttypes.Height,
timeoutTimestamp uint64,
) error {
if !k.GetSendEnabled(ctx) {
Copy link
Member

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!

@mergify mergify bot merged commit 24b17bd into main Oct 11, 2022
@mergify mergify bot deleted the colin/1918-remove-SendTransfer-pubclicfunc branch October 11, 2022 10:43
mergify bot pushed a commit that referenced this pull request Oct 11, 2022
…with MsgTransfer (#2446)

## Description

closes: #1918

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [x] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [x] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [x] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [x] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [x] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [x] Re-reviewed `Files changed` in the Github PR explorer
- [x] Review `Codecov Report` in the comment section below once CI passes

(cherry picked from commit 24b17bd)

# Conflicts:
#	modules/apps/transfer/keeper/mbt_relay_test.go
#	modules/apps/transfer/keeper/msg_server_test.go
#	modules/apps/transfer/keeper/relay.go
#	modules/apps/transfer/keeper/relay_test.go
crodriguezvega added a commit that referenced this pull request Oct 19, 2022
…with MsgTransfer (backport #2446) (#2527)

* refactor: remove SendTransfer, require IBC transfers to be initiated with MsgTransfer (#2446)

## Description

closes: #1918

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [x] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [x] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [x] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [x] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [x] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [x] Re-reviewed `Files changed` in the Github PR explorer
- [x] Review `Codecov Report` in the comment section below once CI passes

(cherry picked from commit 24b17bd)

# Conflicts:
#	modules/apps/transfer/keeper/mbt_relay_test.go
#	modules/apps/transfer/keeper/msg_server_test.go
#	modules/apps/transfer/keeper/relay.go
#	modules/apps/transfer/keeper/relay_test.go

* fix test

Co-authored-by: colin axnér <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make SendTransfer private
4 participants