-
Notifications
You must be signed in to change notification settings - Fork 624
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
Simplify SendPacket API #1703
Simplify SendPacket API #1703
Conversation
@@ -375,7 +375,9 @@ func (endpoint *Endpoint) SendPacket(packet exported.PacketI) error { | |||
channelCap := endpoint.Chain.GetChannelCapability(packet.GetSourcePort(), packet.GetSourceChannel()) | |||
|
|||
// no need to send message, acting as a module | |||
err := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.SendPacket(endpoint.Chain.GetContext(), channelCap, packet) | |||
// TODO: Change Endpoint SendPacket to take in arguments directly |
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 will address this in a subsequent PR to reduce diffs
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.
Can you open an issue to track this work?
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.
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.
Nice!! Looks super clean, love how much simpler the SendTx
code in ICA is.
Left one question about telemetry, not sure how critical it really is tho.
🚀
telemetry.NewLabel(coretypes.LabelDestinationPort, destinationPort), | ||
telemetry.NewLabel(coretypes.LabelDestinationChannel, destinationChannel), | ||
} | ||
labels := []metrics.Label{} |
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.
Are we losing this telemetry info now for destination port and channel?
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.
Yes. cc: @colin-axner
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 it is okay, so long as there is telemetry within core IBC showing where packets are being sent from/to, then the telemetry could filter on port ID to get the same info. This might break external UX though, so might make more sense to do in a followup
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 don't think telemetry is used on external UX? Also I just think it makes sense to keep it clean here, rather than grabbing channel just so that we can emit the same telemetry.
Afaiu, breaking telemetry does not count as an api breaking or state machine breaking change. So i don't want to be so restrictive and conservative in this case.
I'm in favor of just removing it. We can add it in the changelog so that people who need this information are informed of it beforehand
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.
Changing any of the labels will break the telemetry counter. Thus any IBC apps reporting statistics via Prometheus would likely break. I think it makes sense to check with external apps to ensure we don't make a disruptive change out of the blue
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.
We could remove this telemetry entirely and replace it with one in core IBC that emits per packet sent, rather than duplicating for every application
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 great! Nice cleanup. I just had a few small questions
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.
Thanks, @AdityaSripal! I think we need to add also some documentation in the migration docs for app developers, right? And we should decide in what version this should be released...
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.
Concept ACK, see suggested feedback. Seems like a straightforward change. Nice improvement! 🦾
telemetry.NewLabel(coretypes.LabelDestinationPort, destinationPort), | ||
telemetry.NewLabel(coretypes.LabelDestinationChannel, destinationChannel), | ||
} | ||
labels := []metrics.Label{} |
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 it is okay, so long as there is telemetry within core IBC showing where packets are being sent from/to, then the telemetry could filter on port ID to get the same info. This might break external UX though, so might make more sense to do in a followup
@@ -375,7 +375,9 @@ func (endpoint *Endpoint) SendPacket(packet exported.PacketI) error { | |||
channelCap := endpoint.Chain.GetChannelCapability(packet.GetSourcePort(), packet.GetSourceChannel()) | |||
|
|||
// no need to send message, acting as a module | |||
err := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.SendPacket(endpoint.Chain.GetContext(), channelCap, packet) | |||
// TODO: Change Endpoint SendPacket to take in arguments directly |
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.
Can you open an issue to track this work?
Marking as draft just so it is easier to know when suggestions and merge conflicts have been addressed and the pr is ready for a full review again |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1703 +/- ##
==========================================
+ Coverage 78.65% 78.67% +0.02%
==========================================
Files 178 178
Lines 12367 12311 -56
==========================================
- Hits 9727 9686 -41
+ Misses 2208 2197 -11
+ Partials 432 428 -4
|
@@ -25,7 +26,7 @@ type Keeper struct { | |||
cdc codec.BinaryCodec | |||
paramSpace paramtypes.Subspace | |||
|
|||
ics4Wrapper icatypes.ICS4Wrapper | |||
ics4Wrapper porttypes.ICS4Wrapper |
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.
No reason for us to duplicate ICS4Wrapper in the expected keepers. the interface already exists in port types. This also prevents accidentally missing a function. Done for all apps
@@ -20,7 +20,7 @@ import ( | |||
var ( | |||
disabledTimeoutTimestamp = uint64(0) | |||
disabledTimeoutHeight = clienttypes.ZeroHeight() | |||
timeoutHeight = clienttypes.NewHeight(1, 100) | |||
defaultTimeoutHeight = clienttypes.NewHeight(1, 100) |
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 had to change this back since it was conflicting with the local var in TestSendPacket
. I also just find it makes more sense, since there are some non default values I put in the test 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.
There's a function on the destination chain now. suite.chainB.GetTimeoutHeight()
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 added back telemetry. Would prefer to intentionally remove it in a followup pr
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.
LGTM! Nice work 🚀
timeoutHeight clienttypes.Height, | ||
timeoutTimestamp uint64, | ||
data []byte, | ||
) (sequence uint64, err error) |
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.
Should we leave these as unnamed return args as they are in other methods such as GetAppVersion
?
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.
It is good as is in my opinion. OnChanOpenInit has named returns. GetAppVersion probably should have had a named return
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.
Maybe we can make them all consistent in housekeeping followup.
Co-authored-by: Damian Nolan <[email protected]>
…o aditya/send-packet-api
@crodriguezvega Are there any changes left you think we should make? With regards to your current suggestions, I think it probably makes sense to continue using the packet variables so as to ensure we are referencing the correct values Otherwise I think this pr is ready to be merged |
@@ -238,10 +238,21 @@ DecodePacketData(encoded []byte) (CustomPacketData) { | |||
Then a module must encode its packet data before sending it through IBC. |
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 realized that this file is actually not used in the docs website, so could probably be removed...
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 see, looks like we took this info and split it up into separate files? Lets open an issue whose task is to see if any info in here should be moved before deleting the file
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.
Yes, I just updated the documentation. I would appreciate a review to check it all looks ok. After that, I agree we can merge it. |
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.
Thanks @crodriguezvega for updating docs! I re-reviewed all changes
## Description closes: #1395 --- 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. - [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing) - [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`) - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] 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 (cherry picked from commit 88525d2) # Conflicts: # modules/apps/29-fee/keeper/keeper.go # modules/core/04-channel/keeper/packet.go # modules/core/04-channel/keeper/packet_test.go
* Simplify SendPacket API (#1703) ## Description closes: #1395 --- 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. - [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing) - [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`) - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] 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 (cherry picked from commit 88525d2) # Conflicts: # modules/apps/29-fee/keeper/keeper.go # modules/core/04-channel/keeper/packet.go # modules/core/04-channel/keeper/packet_test.go * fix conflicts * Update develop.md * remove test that was added for 02-client refactor (and therefore before v6.0.x was branched off) Co-authored-by: Aditya <[email protected]> Co-authored-by: crodriguezvega <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: colin axnér <[email protected]>
Description
closes: #1395
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