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

Adding GetChannelsWithPortPrefix function to 04-channel #2304

Merged

Conversation

chatton
Copy link
Contributor

@chatton chatton commented Sep 19, 2022

Description

closes: #2245


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

@@ -31,7 +33,7 @@ func (m Migrator) AssertChannelCapabilityMigrations(ctx sdk.Context) error {

isAuthenticated := m.keeper.scopedKeeper.AuthenticateCapability(ctx, cap, name)
if !isAuthenticated {
return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityNotOwned, "expected capability owner: %s", types.SubModuleName)
return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityNotOwned, "expected capability owner: %s", commitmenttypes.SubModuleName)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how this ended up as commitmenttypes I see the import previously would've been incorrect also.
It should be controllertypes.SubModuleName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, I was wondering about that actually, I changed the import to be inline with other imports in the file. I can update this now.

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 the dangers of types as the import alias.


// filteredPortPrefix returns the prefix key for the given port prefix.
func filteredPortPrefix(portPrefix string) []byte {
return []byte(fmt.Sprintf("%s/ports/%s", host.KeyChannelEndPrefix, portPrefix))
Copy link
Member

Choose a reason for hiding this comment

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

small nit:

Suggested change
return []byte(fmt.Sprintf("%s/ports/%s", host.KeyChannelEndPrefix, portPrefix))
return []byte(fmt.Sprintf("%s/%s/%s", host.KeyChannelEndPrefix, host.KeyPortPrefix, portPrefix))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call

name := host.ChannelCapabilityPath(ch.PortId, ch.ChannelId)
cap, found := m.keeper.scopedKeeper.GetCapability(ctx, name)
capacity, found := m.keeper.scopedKeeper.GetCapability(ctx, name)
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 renamed this to avoid shadowing the built in cap function.

@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2022

Codecov Report

Merging #2304 (2ffd5e4) into main (b97729d) will decrease coverage by 0.08%.
The diff coverage is 67.18%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2304      +/-   ##
==========================================
- Coverage   79.54%   79.45%   -0.09%     
==========================================
  Files         175      175              
  Lines       12069    12149      +80     
==========================================
+ Hits         9600     9653      +53     
- Misses       2045     2067      +22     
- Partials      424      429       +5     
Impacted Files Coverage Δ
...27-interchain-accounts/controller/keeper/events.go 0.00% <ø> (ø)
...nterchain-accounts/controller/keeper/grpc_query.go 100.00% <ø> (ø)
...nterchain-accounts/controller/keeper/msg_server.go 100.00% <ø> (ø)
...27-interchain-accounts/controller/keeper/params.go 100.00% <ø> (ø)
.../27-interchain-accounts/controller/keeper/relay.go 73.33% <ø> (ø)
...in-accounts/controller/migrations/v5/migrations.go 89.65% <ø> (ø)
...ps/27-interchain-accounts/controller/types/msgs.go 92.30% <ø> (ø)
...ps/27-interchain-accounts/genesis/types/genesis.go 0.00% <ø> (ø)
...les/apps/27-interchain-accounts/host/ibc_module.go 95.00% <ø> (ø)
.../apps/27-interchain-accounts/host/keeper/events.go 0.00% <ø> (ø)
... and 116 more

@@ -469,3 +488,8 @@ func (k Keeper) iterateHashes(_ sdk.Context, iterator db.Iterator, cb func(portI
}
}
}

// filteredPortPrefix returns the prefix key for the given port prefix.
func filteredPortPrefix(portPrefix string) []byte {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to place this function in types/keys.go instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good suggestion, done!

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

LGTM, just would like a little testing in 04-channel as well. Nice work thus far!

modules/core/04-channel/keeper/keeper.go Outdated Show resolved Hide resolved
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.

Looks good to me! Left a couple of nits and a question regarding naming.

Great work! ⭐

Comment on lines 26 to 29
portIdWithOutPrefix := ibctesting.MockPort
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.SetChannel(suite.chainA.GetContext(), portIdWithOutPrefix, ibctesting.FirstChannelID, channeltypes.Channel{
ConnectionHops: []string{ibctesting.FirstConnectionID},
})
Copy link
Member

Choose a reason for hiding this comment

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

We normally favour using ID over Id in naming (except proto generated code).

Suggested change
portIdWithOutPrefix := ibctesting.MockPort
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.SetChannel(suite.chainA.GetContext(), portIdWithOutPrefix, ibctesting.FirstChannelID, channeltypes.Channel{
ConnectionHops: []string{ibctesting.FirstConnectionID},
})
portIDWithOutPrefix := ibctesting.MockPort
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.SetChannel(suite.chainA.GetContext(), portIDWithOutPrefix, ibctesting.FirstChannelID, channeltypes.Channel{
ConnectionHops: []string{ibctesting.FirstConnectionID},
})

Copy link
Contributor

Choose a reason for hiding this comment

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

We only favor "Id" in proto naming because using "ID" messes with the grpc gateway generation (I'm not sure why nor if this is still an issue, but it was when we initially switched to proto)

@@ -384,6 +384,24 @@ func (k Keeper) IterateChannels(ctx sdk.Context, cb func(types.IdentifiedChannel
}
}

// GetChannelsWithPortPrefix returns all channels with the specified port prefix.
func (k Keeper) GetChannelsWithPortPrefix(ctx sdk.Context, portPrefix string) []types.IdentifiedChannel {
Copy link
Member

Choose a reason for hiding this comment

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

Should we consider naming this function to align with GetAllChannels below which returns []types.IdentifiedChannel also?

Maybe GetAllChannelsWithPortPrefix or GetAllPortPrefixedChannels?

I don't feel particularly strongly but just a thought. cc. @crodriguezvega @colin-axner

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 GetAllChannelsWithPortPrefix to stay consistent

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me. I have no preference

Comment on lines +390 to +392
if strings.TrimSpace(portPrefix) == "" {
return k.GetAllChannels(ctx)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

missed this condition before the tests!

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! This is a good addition

…ns.go

Co-authored-by: Damian Nolan <damiannolan@gmail.com>
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

LGTM! Left some suggestions for the tests, mostly nits. Happy to go with what you think would be best

Comment on lines +390 to +392
if strings.TrimSpace(portPrefix) == "" {
return k.GetAllChannels(ctx)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! This is a good addition

modules/core/04-channel/keeper/keeper_test.go Outdated Show resolved Hide resolved
modules/core/04-channel/keeper/keeper_test.go Outdated Show resolved Hide resolved
modules/core/04-channel/keeper/keeper_test.go Outdated Show resolved Hide resolved
modules/core/04-channel/keeper/keeper_test.go Show resolved Hide resolved

// containsAll verifies if all elements in the expected slice exist in the actual slice
// independent of order.
func containsAll(expected, actual []types.IdentifiedChannel) bool {
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 added this function as I saw that the order of the values returned in GetAllChannels is not necessarily in the order they were added.

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Followup ACK

@chatton chatton enabled auto-merge (squash) September 29, 2022 11:37
Comment on lines +94 to +98
allChannels := []types.IdentifiedChannel{
types.NewIdentifiedChannel(transfertypes.PortID, ibctesting.FirstChannelID, types.Channel{}),
types.NewIdentifiedChannel(differentChannelPortID, secondChannelID, types.Channel{}),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great suggestion @colin-axner , this makes things much cleaner.

@chatton chatton merged commit ce5aba8 into main Sep 29, 2022
@chatton chatton deleted the cian/issue#2245-add-port-prefix-iterator-function-to-04-channel branch September 29, 2022 15:40
mergify bot pushed a commit that referenced this pull request Sep 29, 2022
(cherry picked from commit ce5aba8)

# Conflicts:
#	CHANGELOG.md
damiannolan pushed a commit that referenced this pull request Sep 30, 2022
…) (#2445)

* Adding GetChannelsWithPortPrefix function to 04-channel (#2304)

(cherry picked from commit ce5aba8)

# Conflicts:
#	CHANGELOG.md

* fix conflict

* Update CHANGELOG.md

Co-authored-by: Cian Hatton <cian@interchain.io>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
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.

Add port prefix iterator function to 04-channel
5 participants