Skip to content

fix: add bigint replacer to maths json stringify #70

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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

Aerilym
Copy link
Collaborator

@Aerilym Aerilym commented May 27, 2025

This pull request introduces a utility function for JSON stringification with BigInt support and integrates it into existing error handling in the apps/staking/tests/maths.spec.ts file. The changes improve the handling of BigInt values during debugging and error reporting.

Utility Function Addition:

  • Added jsonBigIntReplacer import from @session/util-js/bigint.
  • Introduced a jsonStringify helper function that uses JSON.stringify with jsonBigIntReplacer to handle BigInt values.

Error Handling Improvement:

  • Updated error message in the parseContributorDetails fuzzing test to use jsonStringify for better representation of BigInt values in the contributors object.

@Copilot Copilot AI review requested due to automatic review settings May 27, 2025 00:39
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves BigInt handling in error reporting for the maths module by introducing a JSON stringification helper that uses a custom BigInt replacer.

  • Imported jsonBigIntReplacer to handle BigInt values.
  • Introduced jsonStringify helper for improved error message formatting.
  • Integrated jsonStringify in the error handling within the parseContributorDetails fuzzing test.

@@ -22,6 +24,8 @@ function createContributor(amount: bigint): Contributor {
};
}

const jsonStringify = (value:unknown) => JSON.stringify(value, jsonBigIntReplacer);
Copy link
Preview

Copilot AI May 27, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider adding a brief comment explaining the purpose of the jsonStringify helper and its relation to handling BigInt values, to improve code maintainability and clarity.

Copilot uses AI. Check for mistakes.

@@ -207,7 +211,7 @@ describe(`parseContributorDetails fuzzing (fuzzAmount: ${fuzzAmount}`, () => {
minStake = res.minStake;
totalStaked = res.totalStaked;
} catch (e) {
throw new Error(`Failed to get min stake for ${JSON.stringify(contributors)} ${e}`);
throw new Error(`Failed to get min stake for ${jsonStringify(contributors)} ${e}`);
Copy link
Preview

Copilot AI May 27, 2025

Choose a reason for hiding this comment

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

[nitpick] Verify that the enhanced error message provides sufficient context for debugging; consider logging additional relevant details if future debugging proves challenging.

Suggested change
throw new Error(`Failed to get min stake for ${jsonStringify(contributors)} ${e}`);
throw new Error(
`Failed to get min stake during fuzz test iteration ${i}/${fuzzAmount}. ` +
`Contributors: ${jsonStringify(contributors)}. ` +
`Error: ${e}. ` +
`State: { minStake: ${minStake ?? 'undefined'}, totalStaked: ${totalStaked ?? 'undefined'} }`
);

Copilot uses AI. Check for mistakes.

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.

1 participant