Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Missing test(s) for message recovery #9137

Merged
merged 8 commits into from
Nov 23, 2023

Conversation

sitetester
Copy link
Contributor

What was the problem?

This PR resolves #8193

How was it solved?

Update code since LIP is updated

How was it tested?

Corresponding tests added

@sitetester sitetester changed the base branch from development to release/6.1.0 November 6, 2023 17:23
Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Merging #9137 (bc3dc83) into release/6.1.0 (7d8301a) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff               @@
##           release/6.1.0    #9137   +/-   ##
==============================================
  Coverage          84.35%   84.35%           
==============================================
  Files                655      655           
  Lines              24113    24113           
  Branches            3497     3497           
==============================================
  Hits               20341    20341           
  Misses              3772     3772           
Files Coverage Δ
...roperability/mainchain/commands/recover_message.ts 99.38% <ø> (ø)

@sitetester sitetester marked this pull request as ready for review November 7, 2023 10:13
@sitetester sitetester requested review from shuse2 and gkoumout November 7, 2023 10:14
Comment on lines 166 to 173

// Check that sending chain is valid. This check is relevant only in case direct sidechain channels are enabled
if (!chainID.equals(getMainchainID(chainID)) && !ccm.sendingChainID.equals(chainID)) {
return {
status: VerifyStatus.FAIL,
error: new Error('Cross-chain message sending chain ID is not valid.'),
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

im not sure why this is added 🤔

Copy link

Choose a reason for hiding this comment

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

This check is not needed in the mainchain. As the comment in the LIP says

        # Check that sending chain is valid. This check is relevant only in case direct sidechain channels are enabled
        if OWN_CHAIN_ID != getMainchainID() and ccm.sendingChainID != OWN_CHAIN_ID:
            raise  Exception("Cross-chain message sending chain ID is not valid.")

Currently (no direct channels) the message recovery command exists only in mainchain and this extra check is not needed. It is anyways pointless since the OWN_CHAIN_ID != getMainchainID() would never be true in mainchain, so adding it would never change the outcome of the verification.

In the future, in case direct sidechain channels get enabled, then the message recovery command would also need to be supported in the sidechains. There, we would need this check to ensure the validity (this check essentially says that sidechains can never forward messages from other chains).

So this check would be needed only in case message recovery gets enabled in sidechains.

@sitetester sitetester changed the title Verify message recovery Update message recovery Nov 8, 2023
@sitetester sitetester self-assigned this Nov 8, 2023
@sitetester sitetester changed the title Update message recovery Missing test(s) for message recovery Nov 9, 2023
@sitetester sitetester requested a review from shuse2 November 9, 2023 15:34
@sitetester sitetester requested review from AndreasKendziorra and removed request for gkoumout and AndreasKendziorra November 16, 2023 09:54
Copy link
Contributor

@AndreasKendziorra AndreasKendziorra left a comment

Choose a reason for hiding this comment

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

All changes look good, but there is one open point in issue #8193 , namely the very last one:

Tests of lines 594, 633: I am not sure that it is checked that recoveredCCMs array is correctly updated.

The tests were refactored quite a bit in the meantime, but the corresponding ones in the current version are the ones in line 572 and 603. So far, it is only tested that terminatedOutboxStore.set is called, but is not tested that it is called with the correct value. So I think the right way to do this is

  1. testing that RMTCalculateRoot was called with the right values (namely recoveredCCMs as Greg commented), and
  2. that terminatedOutboxStore.set is called with the return value of RMTCalculateRoot.

@@ -584,25 +591,53 @@ describe('MessageRecoveryCommand', () => {
),
};

const recoveredCCM = await command['_applyRecovery'](ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

due to this, the expectation in line 597 will be met, even though it was not triggered from the function you are testing, right? Alternatively, you could create a copy of ctx, change the status of ccm to CCM_STATUS_CODE_RECOVERED and swap ccm.sendingChainID and ccm.receivingChainID.

@@ -613,23 +648,41 @@ describe('MessageRecoveryCommand', () => {
),
};

const recoveredCCM = await command['_forwardRecovery'](ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@ishantiw ishantiw merged commit 8c7aba6 into release/6.1.0 Nov 23, 2023
10 checks passed
@ishantiw ishantiw deleted the 8193-verify-message-recovery branch November 23, 2023 16:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit Test Review: Interoperability commands
5 participants