refactor: replace local utility functions with CCC equivalents#19
refactor: replace local utility functions with CCC equivalents#19
Conversation
…t all call sites - Replace max() with Number(ccc.numMax()) at 2 call sites (entities.ts, codec.ts) - Replace gcd() with ccc.gcd() at 1 call site (entities.ts) - Replace hexFrom(entity) with entity.toHex() at 4 call sites (order.ts, sdk.ts) - Replace hexFrom(bytes) with ccc.hexFrom() at 1 call site (faucet main.ts) - Remove max/gcd/hexFrom from @ickb/utils imports in all modified files
…e() internals - Delete max, min, gcd, hexFrom, isHex from @ickb/utils - Update unique() to use i.toHex() instead of local hexFrom(i) - Preserve all iCKB-unique utilities (binarySearch, asyncBinarySearch, shuffle, collect, BufferedGenerator, MinHeap, sum) - Add changeset documenting the breaking API removal - pnpm check:full passes with zero errors
- Create 02-01-SUMMARY.md documenting utility function replacement - Update STATE.md: Phase 02 complete, plan 1/1 done, decisions recorded - Update ROADMAP.md: Phase 2 marked complete with 1/1 plans done - Update REQUIREMENTS.md: DEDUP-01 through DEDUP-05 marked complete
Summary of ChangesHello @phroi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the codebase by removing redundant utility functions from Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a great step towards aligning the codebase with CCC by removing duplicated utility functions. The refactoring is well-documented through extensive planning and research files. The changes in most files correctly replace local utilities with their CCC equivalents (ccc.gcd, entity.toHex, ccc.hexFrom). However, I've found two instances where Math.max() was used instead of the documented choice of Number(ccc.numMax()). Using the CCC-provided function would ensure consistency with the PR's stated goal. Please see the specific comments for details.
ccc.numMax() converts number args to bigint and returns bigint, requiring a Number() wrapper to convert back. These call sites operate entirely on number types, so Math.max() is the correct replacement for the old generic max<T>(). Also downgrade @ickb/order and @ickb/sdk changeset bumps from major to patch — the only change in those packages is an internal swap of one standard-library call for another, with no public API change.
The implementation was changed from hexFrom()/toBytes() to toHex(), but the JSDoc still referenced "byte representation" and mol.Entity.
9333d26 to
a4015fc
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request completes Phase 2 of the project roadmap, focusing on 'CCC Utility Adoption'. It involves replacing five local utility functions (max, min, gcd, hexFrom, isHex) in @ickb/utils with their CCC equivalents or native JavaScript functions, and subsequently deleting the local implementations. Specifically, max() and min() are replaced with Math.max() and Math.min() for number-typed contexts to avoid unnecessary bigint conversions, gcd() is replaced with ccc.gcd(), hexFrom(entity) calls are updated to entity.toHex(), and hexFrom(bytes) calls are updated to ccc.hexFrom(). The internal implementation of the unique() utility function is also updated to use entity.toHex(). All relevant planning documents (REQUIREMENTS.md, ROADMAP.md, STATE.md, and phase-specific plan/summary/research/verification files) have been updated to reflect the completion of these tasks and the rationale behind specific implementation choices, such as using Math.max() over ccc.numMax() and entity.toHex() over ccc.hexFrom(entity.toBytes()). A changeset entry has been added to document the breaking API changes in @ickb/utils.
|
LGTM Phroi %74 |
Why
The local
max,min,gcd,hexFrom, andisHexutilities in@ickb/utilsduplicate functionality now provided by CCC. Removing them reduces maintenance surface and aligns the codebase with upstream.Changes
max,min,gcd,hexFrom,isHexfrom@ickb/utilspublic APIgcd()withccc.gcd()andmax()withMath.max()at call sites operating onnumbertypeshexFrom(entity)withentity.toHex()andhexFrom(bytes)withccc.hexFrom()unique()internals to usetoHex()directly@ickb/utils,@ickb/order,@ickb/sdk