Skip to content

Multi payload packet#8545

Merged
AdityaSripal merged 12 commits intomainfrom
aditya/multi-payload
Jun 23, 2025
Merged

Multi payload packet#8545
AdityaSripal merged 12 commits intomainfrom
aditya/multi-payload

Conversation

@AdityaSripal
Copy link
Copy Markdown
Contributor

@AdityaSripal AdityaSripal commented Jun 18, 2025

Description

closes: IBCGO-2371


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.

  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Include changelog entry when appropriate (e.g. chores should be omitted from changelog).
  • Wrote unit and integration tests if relevant.
  • Updated documentation (docs/) if anything is changed.
  • Added godoc comments if relevant.
  • Self-reviewed Files changed in the GitHub PR explorer.
  • Provide a conventional commit message to follow the repository standards.

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 18, 2025

Codecov Report

Attention: Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.

Project coverage is 57.63%. Comparing base (5f8fd48) to head (933c996).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
modules/core/04-channel/v2/keeper/msg_server.go 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8545      +/-   ##
==========================================
+ Coverage   57.58%   57.63%   +0.05%     
==========================================
  Files         317      317              
  Lines       22666    22668       +2     
==========================================
+ Hits        13052    13065      +13     
+ Misses       9006     8998       -8     
+ Partials      608      605       -3     
Flag Coverage Δ
08-wasm 65.99% <ø> (ø)
e2e 1.13% <ø> (ø)
ibc-go 63.06% <93.54%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AdityaSripal AdityaSripal marked this pull request as ready for review June 19, 2025 15:32
@linear
Copy link
Copy Markdown

linear bot commented Jun 21, 2025

@gjermundgaraba gjermundgaraba linked an issue Jun 21, 2025 that may be closed by this pull request
@gjermundgaraba gjermundgaraba requested a review from Copilot June 21, 2025 20:55
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR expands the packet handling functionality to support sending multiple payloads atomically in the same IBC packet. Key changes include:

  • Adding support for asynchronous payloads via a new NewAsyncMockPayload function and handling of async packet result status in the mock IBC module.
  • Refactoring endpoint methods (MsgSendPacket and MsgSendPacketWithSender) to accept variadic payload parameters for multiple payload support.
  • Updating tests and keeper logic to validate and manage multiple payloads as well as updating error messaging for payload validation.

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
testing/mock/v2/mock.go Introduces NewAsyncMockPayload to support async payloads.
testing/mock/v2/ibc_module.go Updates OnRecvPacket to detect async payloads and return corresponding status.
testing/endpoint_v2.go Refactors MsgSendPacket and MsgSendPacketWithSender to support multiple payloads.
modules/core/04-channel/v2/types/* Adjusts payload validation and error messages to reflect multi-payload support.
modules/core/04-channel/v2/keeper/* and msg_server.* Updates acknowledgement, packet, and message server logic to handle multiple payloads.
modules/apps/transfer/v2/transfer_test.go Adds tests for transfer flows with multiple payloads.
CHANGELOG.md Adds changelog entry documenting multi-payload packet support.
Comments suppressed due to low confidence (2)

testing/mock/v2/mock.go:38

  • [nitpick] Consider adding a comment documenting NewAsyncMockPayload to clarify its purpose, its behavior, and how it differs from NewMockPayload and NewErrorMockPayload.
func NewAsyncMockPayload(sourcePort, destPort string) channeltypesv2.Payload {

testing/endpoint_v2.go:24

  • [nitpick] Update the function documentation for MsgSendPacket to clearly specify that it now accepts multiple payloads and describe the expected behavior when multiple payloads are provided.
func (endpoint *Endpoint) MsgSendPacket(timeoutTimestamp uint64, payloads ...channeltypesv2.Payload) (channeltypesv2.Packet, error) {

@gjermundgaraba
Copy link
Copy Markdown
Contributor

bugbot run

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

✅ BugBot reviewed your changes and found no bugs!


BugBot free trial expires on July 22, 2025
You have used $0.00 of your $50.00 spend limit so far. Manage your spend limit in the Cursor dashboard.

Was this report helpful? Give feedback by reacting with 👍 or 👎

Copy link
Copy Markdown
Contributor

@gjermundgaraba gjermundgaraba left a comment

Choose a reason for hiding this comment

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

This looks very good! I left a couple of nits and minor cleanups with the tests. Feel free to merge if you agree with these and get the fixed 👍

}

// Validate the acknowledgement against the payload length
if ack.Success() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: could be a single if statement

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I left as is, because i find it more readable as to why the check exists

var (
path *ibctesting.Path
packet types.Packet
payload types.Payload
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this does anything. There is already a payload in the test function itself that is set with payload := ....

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it's never going to change, but you need access to it, you could just set it once at the top and use it as you do already. But right now I think it will always be empty.

var (
packet types.Packet
ack types.Acknowledgement
packet types.Packet
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In this one, it does get set in the test function itself, but as far as I can tell, it never changes, so you could just set it once here and use it directly and not reset it every test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The payloads are getting modulated in the test cases

var (
path *ibctesting.Path
packet types.Packet
payload types.Payload
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same deal here, I believe


func (s *TypesTestSuite) TestMsgSendPacketValidateBasic() {
var msg *types.MsgSendPacket
var payload types.Payload
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one doesn't look like it's used in the test function itself, so probably don't need to define it here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It gets used in the test cases

@AdityaSripal AdityaSripal merged commit 38dd181 into main Jun 23, 2025
52 checks passed
@AdityaSripal AdityaSripal deleted the aditya/multi-payload branch June 23, 2025 12:45
@AdityaSripal AdityaSripal restored the aditya/multi-payload branch June 25, 2025 11:15
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.

Multi-payload support

3 participants