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

_verifyLivenessConditionForRegisteredChains & verifyLivenessConditionForRegisteredChains have duplicated code #8731

Closed
sitetester opened this issue Jul 13, 2023 · 1 comment
Assignees

Comments

@sitetester
Copy link
Contributor

sitetester commented Jul 13, 2023

Description

// framework/src/modules/interoperability/mainchain/commands/submit_mainchain_cross_chain_update.ts

private _verifyLivenessConditionForRegisteredChains(
	context: CommandVerifyContext<CrossChainUpdateTransactionParams>,
): void {
	const certificate = codec.decode<Certificate>(certificateSchema, context.params.certificate);
	validateCertificate(certificate);

	if (context.header.timestamp - certificate.timestamp > LIVENESS_LIMIT / 2) {
		throw new Error(
			`The first CCU with a non-empty inbox update cannot contain a certificate older than ${
				LIVENESS_LIMIT / 2
			} seconds.`,
		);
	}
}

vs

// framework/src/modules/interoperability/utils.ts

export const verifyLivenessConditionForRegisteredChains = (
	ccu: CrossChainUpdateTransactionParams,
	blockTimestamp: number,
) => {
	const certificate = codec.decode<Certificate>(certificateSchema, ccu.certificate);
	validateCertificate(certificate);

	const limitSecond = LIVENESS_LIMIT / 2;
	if (blockTimestamp - certificate.timestamp > limitSecond) {
		throw new Error(
			`The first CCU with a non-empty inbox update cannot contain a certificate older than ${limitSecond} seconds.`,
		);
	}
};

These two functions have same logic in their bodies. We can merge them together with a modified signature like this:

export const verifyLivenessConditionForRegisteredChains = (
	blockTimestamp: number,
        certificate: Buffer)
) => {

Motivation

  • There is no point to have separate functions for same logic. If we need to change something, either we will do it at both places or it would result in inconsistency.
  • Separate functions means separate test cases (along with preparing their fixtures). It's a waste of time & requires extra effort on behalf of author & reviewer to manage things.

Additional Information

Already mentioned

@Phanco
Copy link
Contributor

Phanco commented Jul 14, 2023

Duplicate issue, #8552.

Can close this issue

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants