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.

Sign up for GitHub

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

Jump to bottom

Add NoForwarding and EmptyForwardingPacketData helper function to use… #6774

Merged
merged 6 commits into from
Jul 8, 2024
Merged

Add NoForwarding and EmptyForwardingPacketData helper function to use… #6774

merged 6 commits into from
Jul 8, 2024

Conversation

hakuno2000
Copy link
Contributor

… in tests

Description

closes: #6723

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 the 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 and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/).
  • Added relevant godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

@DimitrisJim
Copy link
Contributor

thanks! we should add these to testing/values.go and use them in the tests that currently reference them. Since Forwarding can be both nil and empty, NoForwarding should just be set to nil there. (and these probably don't need to be functions now that I think of it! 😄)

crodriguezvega
crodriguezvega approved these changes Jul 8, 2024
View reviewed changes
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.

Thank you for the PR, @hakuno2000! Changes look good to me; I pushed a commit using the new functions in a few more places.

@hakuno2000
Copy link
Contributor Author

Thank you for the PR, @hakuno2000! Changes look good to me; I pushed a commit using the new functions in a few more places.

Thank you so much sir.

crodriguezvega
crodriguezvega reviewed Jul 8, 2024
View reviewed changes
modules/apps/transfer/internal/packet_test.go
@@ -43,7 +41,7 @@ func TestUnmarshalPacketData(t *testing.T) {
Denom: types.NewDenom("atom", types.NewHop("transfer", "channel-0")),
Amount: "1000",
},
}, sender, receiver, "", emptyForwardingPacketData)
}, sender, receiver, "", types.ForwardingPacketData{})
Copy link
Contributor

Choose a reason for hiding this comment

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

I couldn't use the testing value because it was complaining about some import cycle...

Copy link
Contributor

Choose a reason for hiding this comment

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

that's weird. I'm not sure I see why on top of my head.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I also was surprised, but decided to skip this rabbit hole...

DimitrisJim
DimitrisJim approved these changes Jul 8, 2024
View reviewed changes
Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the pick-up hakuno!

@crodriguezvega crodriguezvega merged commit 45342f2 into cosmos:main Jul 8, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crodriguezvega crodriguezvega crodriguezvega approved these changes

@DimitrisJim DimitrisJim DimitrisJim approved these changes

@colin-axner colin-axner Awaiting requested review from colin-axner

@AdityaSripal AdityaSripal Awaiting requested review from AdityaSripal AdityaSripal is a code owner

@damiannolan damiannolan Awaiting requested review from damiannolan damiannolan is a code owner

@srdtrk srdtrk Awaiting requested review from srdtrk srdtrk is a code owner

@chatton chatton Awaiting requested review from chatton

@gjermundgaraba gjermundgaraba Awaiting requested review from gjermundgaraba gjermundgaraba is a code owner

@bznein bznein Awaiting requested review from bznein

Assignees

@crodriguezvega crodriguezvega

@DimitrisJim DimitrisJim

Labels
None yet
Projects
ibc-go
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a NoForwarding helper function to use in tests
3 participants
@hakuno2000 @DimitrisJim @crodriguezvega