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

Add empty signature option - Closes #64670 #6474

Merged
merged 2 commits into from
Jun 9, 2021

Conversation

shuse2
Copy link
Collaborator

@shuse2 shuse2 commented Jun 8, 2021

What was the problem?

This PR resolves #6470

How was it solved?

  • Add numberOfEmptySignatures option to specify number of empty signatures to include in calculation

How was it tested?

  • Add test case where signatures need to include empty signature

@shuse2 shuse2 self-assigned this Jun 8, 2021
readonly minFeePerByte?: number;
readonly baseFees?: BaseFee[];
readonly numberOfSignatures?: number;
readonly numberOfEmptySignatures?: number;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we don't use signatures param in this function, so we need to know how many is required and how many signatures are empty

@@ -37,14 +38,18 @@ const computeTransactionMinFee = (
trx: Record<string, unknown>,
options?: Options,
): bigint => {
const mockSignatures = new Array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it mock? maybe name it emptySignatures?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's both 64bytes and 0 bytes, so empty is not correct I think

Copy link
Contributor

Choose a reason for hiding this comment

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

dummySignatures?

Copy link
Contributor

@mitsuaki-u mitsuaki-u left a comment

Choose a reason for hiding this comment

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

LGTM

@ManuGowda ManuGowda merged commit 67ee094 into hotfix/5.1.1 Jun 9, 2021
@ManuGowda ManuGowda deleted the 6470-fix_min_fee_calc branch June 9, 2021 09:12
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.

3 participants