Skip to content

E2E: Fee middleware counterparty payee not set#1746

Merged
chatton merged 22 commits into
mainfrom
cian/issue#1686-fee-middleware-counterparty-payee-not-set
Aug 10, 2022
Merged

E2E: Fee middleware counterparty payee not set#1746
chatton merged 22 commits into
mainfrom
cian/issue#1686-fee-middleware-counterparty-payee-not-set

Conversation

@chatton

@chatton chatton commented Jul 20, 2022

Copy link
Copy Markdown
Contributor

Description

closes: #1686


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

@chatton chatton marked this pull request as ready for review July 20, 2022 15:56
@chatton chatton mentioned this pull request Jul 26, 2022
3 tasks

@crodriguezvega crodriguezvega left a comment

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.

Nice, @chatton!

Comment thread e2e/fee_middleware_test.go Outdated
packetId := channeltypes.NewPacketId(channelA.PortID, channelA.ChannelID, 1)
packetFee := feetypes.NewPacketFee(testFee, chainAWallet.Bech32Address(chainA.Config().Bech32Prefix), nil)

t.Run("should succeed", func(t *testing.T) {

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.

Maybe something extra in the description to make it more explicit? Like pay packet fee should succeed?

@chatton chatton Aug 8, 2022

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.

based on the nesting, the resulting message will look like TestPayPacketFeeAsync_SingleSender_NoCounterPartyAddress/pay_packet_fee/should_succeed

@colin-axner colin-axner left a comment

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.

Great work! Looks super clean :)

Comment thread e2e/fee_middleware_test.go Outdated
s.Require().Empty(packets)
})

packetId := channeltypes.NewPacketId(channelA.PortID, channelA.ChannelID, 1)

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.

Suggested change
packetId := channeltypes.NewPacketId(channelA.PortID, channelA.ChannelID, 1)
packetId := channeltypes.NewPacketId(chainATx.Packet.GetSourcePort(), chainATx.Packet.GetSourceChannel(), chainATx.Packet.GetSequence())

I think at least modifying the sequence to use the returned chainATx.Packet.GetSequence makes sense, I have no preference on the other arguments, I just figure this code will be copied a lot so might make more sense to derive the values

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.

ah definitely, good catch.

var err error
payPacketFeeTxResp, err = s.PayPacketFeeAsync(ctx, chainA, chainAWallet, packetId, packetFee)
s.Require().NoError(err)
s.AssertValidTxResponse(payPacketFeeTxResp)

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.

love this helper function ❤️

@seantking seantking left a comment

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.

<3

@chatton chatton merged commit f054208 into main Aug 10, 2022
@chatton chatton deleted the cian/issue#1686-fee-middleware-counterparty-payee-not-set branch August 10, 2022 12:13
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.

E2E: Fee middleware counterparty payee not set

4 participants