Skip to content

test: Add packet test for Axon#330

Merged
jjyr merged 8 commits intomainfrom
packet-test-between-axon-axon
Sep 23, 2023
Merged

test: Add packet test for Axon#330
jjyr merged 8 commits intomainfrom
packet-test-between-axon-axon

Conversation

@jjyr
Copy link
Copy Markdown
Collaborator

@jjyr jjyr commented Sep 14, 2023

Description

Add new ibc test TransferTest.
This test only works with Axon chains. (CKB should be added after the contract refactory).

This test sends a IBC call from chain A to chain B, then checks there is a SendPacket event happend on chain A and a ReceivePacket event happend on chain B.

@jjyr jjyr requested a review from ashuralyk September 14, 2023 07:34
@Flouse Flouse requested review from a team and blckngm September 14, 2023 07:48
@jjyr jjyr force-pushed the packet-test-between-axon-axon branch from 2559325 to 290d84b Compare September 14, 2023 09:31
@ashuralyk
Copy link
Copy Markdown
Contributor

This test only works with Axon chains. (CKB should be added after the contract refactory)

this means to prevent the duplicate work on refactoring test or just cannot adapt for current CKB implementation?

@jjyr
Copy link
Copy Markdown
Collaborator Author

jjyr commented Sep 15, 2023

This test only works with Axon chains. (CKB should be added after the contract refactory)

this means to prevent the duplicate work on refactoring test or just cannot adapt for current CKB implementation?

  1. We need to implement the CKB version MockTransfer.
  2. We need to implement the equavalent version sendTransfer method for CKB contract.
  3. We need refactoring the test to support CKB chains.

@Flouse Flouse requested a review from wenyuanhust September 17, 2023 08:50
@jjyr jjyr force-pushed the packet-test-between-axon-axon branch from 290d84b to 7b2b283 Compare September 18, 2023 02:38
Comment thread tools/ibc-test/src/tests/ibc/transfer.rs Outdated
Comment thread tools/ibc-solidity-abi/src/lib.rs Outdated
Comment thread tools/ibc-test/src/tests/ibc/transfer.rs Outdated
Comment thread tools/ibc-solidity-abi/src/generated/mod.rs Outdated
@jjyr
Copy link
Copy Markdown
Collaborator Author

jjyr commented Sep 20, 2023

@ashuralyk @blckngm

The new commits using ChainDriver#ibc_token_transfer to transfer ICS tokens, please review the updated PR.

  1. Delete a lot of code from TransferTest.
  2. Please check the implementation of ibc_token_transfer function, we could implement CKB in the same way.

@jjyr jjyr force-pushed the packet-test-between-axon-axon branch from ada74f8 to 25ba9ef Compare September 20, 2023 06:25
@jjyr jjyr requested review from ashuralyk and blckngm September 20, 2023 06:28
@Flouse Flouse mentioned this pull request Sep 19, 2023
39 tasks
Comment thread tools/ibc-solidity-abi/src/lib.rs Outdated
@jjyr jjyr force-pushed the packet-test-between-axon-axon branch from c8f79ce to 2a9e93d Compare September 20, 2023 08:08
@jjyr jjyr requested a review from blckngm September 20, 2023 08:10
Comment thread tools/test-framework/src/chain/driver.rs
@ashuralyk
Copy link
Copy Markdown
Contributor

ashuralyk commented Sep 20, 2023

the event log doubled like this:

image

this is because the CI runs two test cases at the same time and we cannot distinguish what errors belong to which one.

NOTE: passed CI tests are not really passed because errors are printed but tests result in success. this problem can be found in our previous merged PRs as well like here.

@jjyr
Copy link
Copy Markdown
Collaborator Author

jjyr commented Sep 21, 2023

NOTE: passed CI tests are not really passed because errors are printed but tests result in success. this problem can be found in our previous merged PRs as well like here.

Not ture, of course these CI are passed, the error messages are raised from monitor thread, becaused we haven't correctly implement the ibc4ckb interfaces.

I have already clarified this known issue from an early PR: #308 (comment)

Known issues:
there are some error output of ibc-tests, the reason is the ibc supervisor consist watching the two chains, but we do not fully implement the CKB relayer.

Please lookup from the error backtrace, it only affects the monitor fetching events operations.

Here is another example failed CI, if the error happend within the test case instead of the monitor, it will exit with a failure status. You need to find the first error output, which is raised from the test thread, not the monitor's error output.

https://github.com/synapseweb3/forcerelay/actions/runs/6182356419/job/16781928376

image

@jjyr jjyr requested a review from ashuralyk September 21, 2023 00:32
@ashuralyk
Copy link
Copy Markdown
Contributor

Not ture, of course these CI are passed, the error messages are raised from monitor thread, becaused we haven't correctly implement the ibc4ckb interfaces.

I also found errors from monitor in Axon endpoint, and the monitor is essential for completing the watch of on-chain events and forward them to Hermes runtime to push the procedure, if it occurs errors, I think the procedure would be stopped.

Here is another example failed CI, if the error happend within the test case instead of the monitor, it will exit with a failure status. You need to find the first error output, which is raised from the test thread, not the monitor's error output.

this problem that I guess is caused by the usage of GLOBAL variables which store the association of IBC connection, channel, and packet with their Axon transactions' hash, I'm not sure whether running two parallel test cases in one instance would access these global variables at same time, which brings CACHE problem.

@jjyr
Copy link
Copy Markdown
Collaborator Author

jjyr commented Sep 21, 2023

the monitor is essential for completing the watch of on-chain events and forward them

I see, I will look into this issue. We could check the monitor thread status and raise an error from the test thread if the monitor is crashed.

I'm not sure whether running two parallel test cases in one instance would access these global variables at same time, which brings CACHE problem.

This is fixed in 412743c by run tests in single thread.


The monitor crash issue is already introduced, so IMO we should fix it in another PR.

Do you think we can merge this PR?

@ashuralyk
Copy link
Copy Markdown
Contributor

the monitor is essential for completing the watch of on-chain events and forward them

I see, I will look into this issue. We could check the monitor thread status and raise an error from the test thread if the monitor is crashed.

I'm not sure whether running two parallel test cases in one instance would access these global variables at same time, which brings CACHE problem.

This is fixed in 412743c by run tests in single thread.


The monitor crash issue is already introduced, so IMO we should fix it in another PR.

Do you think we can merge this PR?

it's ok to me for the merge

sender,
recipient,
token,
None,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is it possible to use timeout as the arg now?

Copy link
Copy Markdown
Contributor

@ashuralyk ashuralyk Sep 22, 2023

Choose a reason for hiding this comment

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

not yet for Axon, it's already under my consideration

Comment thread tools/test-framework/src/chain/ext/transfer.rs
@jjyr jjyr added this pull request to the merge queue Sep 23, 2023
Merged via the queue into main with commit 4205215 Sep 23, 2023
@Flouse Flouse deleted the packet-test-between-axon-axon branch September 24, 2023 03:07
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.

4 participants