Skip to content

idBuilder now accepts either number or bigint#14

Merged
jeffsmale90 merged 1 commit into
mainfrom
fix/id_enforcer
Jun 30, 2025
Merged

idBuilder now accepts either number or bigint#14
jeffsmale90 merged 1 commit into
mainfrom
fix/id_enforcer

Conversation

@jeffsmale90

Copy link
Copy Markdown
Collaborator

📝 Description

Previous idBuilder required the id to be passed as a number type, which restricted the id up to Number.MAX_SAFE_INTEGER.

With this change, the idBuilder now accepts either a bigint or number:

new CaveatBuilder(environment).addCaveat("id", 123n);
new CaveatBuilder(environment).addCaveat("id", 123);

🧪 How to Test?

This is sufficiently covered in unit tests, validating both number and bigint. The above code snippets can be used to manually validate the behaviour.

⚠️ Breaking Changes

List any breaking changes:

  • No breaking changes
  • Breaking changes (describe below):

📋 Checklist

Check off completed items:

  • Code follows the project's coding standards
  • Self-review completed
  • Documentation updated (if needed)
  • Tests added/updated
  • Changelog updated (if needed)
  • All CI checks pass

@jeffsmale90 jeffsmale90 requested a review from a team as a code owner June 27, 2025 00:51
@jeffsmale90 jeffsmale90 requested a review from Copilot June 27, 2025 00:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

Adds support for passing both number and bigint to idBuilder, with corresponding validation and test updates.

  • Extend idBuilder to accept bigint | number and normalize numbers to bigint
  • Update unit tests to import maxUint256, remove obsolete cases, and add a large-bigint scenario
  • Adjust createCaveatBuilder test to use BigInt for the id caveat

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
packages/delegation-toolkit/src/caveatBuilder/idBuilder.ts Expanded parameter type, added type checks/validation
packages/delegation-toolkit/test/caveatBuilder/idBuilder.test.ts Updated tests for bigint support and new edge cases
packages/delegation-toolkit/test/caveatBuilder/createCaveatBuilder.test.ts Switched id test value to BigInt
Comments suppressed due to low confidence (3)

packages/delegation-toolkit/test/caveatBuilder/idBuilder.test.ts:27

  • We cover the negative bigint case, but not a negative number. Add a test like expect(() => buildWithId(-1)).to.throw('Invalid id: must be positive') to ensure number inputs are also validated.
      const invalidId = -1n;

packages/delegation-toolkit/test/caveatBuilder/createCaveatBuilder.test.ts:241

  • The test now only uses a bigint for id. Consider adding a parallel case with a number input to verify createCaveatBuilder handles both types.
      const id = BigInt(Math.floor(Math.random() * 2 ** 32));

packages/delegation-toolkit/src/caveatBuilder/idBuilder.ts:27

  • This runtime check is redundant since idValue is already narrowed to bigint in prior branches. Consider removing to simplify the logic.
  if (typeof idValue !== 'bigint') {

@jeffsmale90 jeffsmale90 merged commit 2f013d6 into main Jun 30, 2025
14 checks passed
@jeffsmale90 jeffsmale90 deleted the fix/id_enforcer branch June 30, 2025 18:47
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.

3 participants