-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Remove IBC from the SDK #8735
Remove IBC from the SDK #8735
Conversation
I will leave the docs for now. Doesn't hurt to have documentation in multiple places |
@@ -114,16 +114,6 @@ func (suite *KeeperTestSuite) TestNewCapability() { | |||
suite.Require().Nil(cap) | |||
} | |||
|
|||
func (suite *KeeperTestSuite) TestOriginalCapabilityKeeper() { |
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.
@AdityaSripal should I replace this test with something? If so, how?
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.
Ahh this is testing the original capability keeper in simapp since that was holding a capability for transfer. Now that simapp no longer does that, we don't need this test.
I would remove it.
@@ -497,83 +436,6 @@ func (s *IntegrationTestSuite) testQueryIBCTx(txRes sdk.TxResponse, cmd *cobra.C | |||
s.Require().Contains(errResp.Error, errMsg) | |||
} | |||
|
|||
// TestLegacyRestErrMessages creates two IBC txs, one that fails, one that |
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.
@AmauryM how should I deal with these tests? Should I open an issue? Comment them out? Replace the IBC parts somehow?
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.
afaik it's only one test. If you're okay to move this one test to the IBC repo, that'd be ideal. If not, then just create an issue and we'll figure out later.
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 there are a couple tests, but I'll look into moving them to the IBC repo and report back
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 managed to move these over to ibc-go but it involved some code duplication since we need to route the suite setup to the ibc-go simapp.
I guess we cannot bring these tests back to the SDK until the SDK supports a Msg type that doesn't support amino and one that doesn't support REST?
someone will need to run proto swagger gen for me. It doesn't work on my machine for some reason |
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.
Looks good colin!
What's the strategy here? Is the idea that master will not import ibc-go
? We're just going to have ibc-go
imports into gaia?
@@ -64,17 +64,6 @@ import ( | |||
"github.com/cosmos/cosmos-sdk/x/gov" | |||
govkeeper "github.com/cosmos/cosmos-sdk/x/gov/keeper" | |||
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" | |||
transfer "github.com/cosmos/cosmos-sdk/x/ibc/applications/transfer" |
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.
Will we be adding these back from the ibc-go repo?
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.
ibc-go has its own simapp now. The SDK team can decide if they would like simapp to import ibc-go (after we do a stable release), but I think it isn't worth it to deal with version dependencies
@@ -114,16 +114,6 @@ func (suite *KeeperTestSuite) TestNewCapability() { | |||
suite.Require().Nil(cap) | |||
} | |||
|
|||
func (suite *KeeperTestSuite) TestOriginalCapabilityKeeper() { |
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.
Ahh this is testing the original capability keeper in simapp since that was holding a capability for transfer. Now that simapp no longer does that, we don't need this test.
I would remove it.
Correct. At least in the short term (until we have releases with ibc-go). It seems possible to me to have a It seems unnecessary to introduce a circular |
@@ -482,7 +482,8 @@ func (suite *AnteTestSuite) TestAnteHandlerFees() { | |||
{ | |||
"signer as enough funds, should pass", | |||
func() { | |||
accNums = []uint64{7} | |||
accNums = []uint64{acc1.GetAccountNumber()} |
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 when I removed IBC registration from simapp, one less account was being created causing the account number to become 6 and not 7. I used the account associated with addr0
since that is the account being funded in this test
Codecov Report
@@ Coverage Diff @@
## master #8735 +/- ##
==========================================
- Coverage 61.39% 59.34% -2.05%
==========================================
Files 672 563 -109
Lines 38404 31251 -7153
==========================================
- Hits 23577 18545 -5032
+ Misses 12342 10551 -1791
+ Partials 2485 2155 -330
|
Hey @colin-axner! Mind updating references in github.com/cosmos/gaia (main branch), please? |
Yes. I presume you mean just for documentation? I cannot update the code until a release of the SDK includes IBC removed. It is my understanding that this is slated for v0.43.0 of the SDK |
Description
The IBC module is being moved to cosmos/ibc-go
closes: #8501
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.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes