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

refactor(tests): use multidenom by default in relay_test #6767

Merged
merged 4 commits into from
Jul 12, 2024

Conversation

DimitrisJim
Copy link
Contributor

@DimitrisJim DimitrisJim commented Jul 4, 2024

Description

first batch for TestSendTransfer and the table OnRecv Is/IsNot Source ones. The ack/timeout ones are main ones left atm.

closes: #XXXX


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.

nil,
},
{
"successful transfer with non-empty forwarding hops and ics20-2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved up

nil,
},
{
"successful transfer of IBC token with memo",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

@@ -162,14 +161,6 @@ func (suite *KeeperTestSuite) TestSendTransfer() {
},
channeltypes.ErrChannelCapabilityNotFound,
},
{
"failure: timeout height and timeout timestamp are zero",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

was testing packet validation logic, this lead to needing to specify the expected values as 100, 100 even though it failed (because it failed after we had set those amounts and since we just directly send message to function, reverting of state doesnt occur)

@@ -214,25 +199,28 @@ func (suite *KeeperTestSuite) TestSendTransfer() {
err = path.RelayPacket(packet)
suite.Require().NoError(err)

// Value that can malleated for Transfer we are testing.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved after msg transfer to create ibc denom on a to avoid confusion

@DimitrisJim DimitrisJim force-pushed the jim/6402-additional-tests-for-multidenom branch 2 times, most recently from 3c8fb4b to 9167213 Compare July 4, 2024 12:58

// denom trace of tokens received on chain B and the associated expected metadata
denomOnB := types.NewDenom(sdk.DefaultBondDenom, types.NewHop(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID))
expDenomMetadataOnB := banktypes.Metadata{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was eye-sore so moved it to separate little func, can add back if people are feeling strongly about this.

@DimitrisJim DimitrisJim force-pushed the jim/6402-additional-tests-for-multidenom branch from 9167213 to 34a6e28 Compare July 4, 2024 13:28
suite.Require().NoError(err) // message committed

packet, err := ibctesting.ParsePacketFromEvents(res.Events)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

completely refactored this. It was initially sending a packet from B to A, relaying it then doing another transfer from A to B. Simplified to match other receive test, namely, send a packet from A to B and then simulate a packet being received on A that has the right trace to signify A being original source.

Copy link
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.

Love the cleanup here!

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.

Great work refactoring these, @DimitrisJim!

@DimitrisJim DimitrisJim force-pushed the jim/6402-additional-tests-for-multidenom branch from 6e03d11 to 3a42ac0 Compare July 12, 2024 12:30
Copy link

@DimitrisJim DimitrisJim merged commit 4c2ec4a into main Jul 12, 2024
67 of 69 checks passed
@DimitrisJim DimitrisJim deleted the jim/6402-additional-tests-for-multidenom branch July 12, 2024 12:46
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.

3 participants