feat(Epoch): transform Epoch into a class and add utility methods
#314
Conversation
🦋 Changeset detectedLatest commit: 63aa8e4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for liveccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for apiccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @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 significantly refactors the Epoch representation within the @ckb-ccc/core package by converting it from a basic array-like structure to a full-fledged TypeScript class. This change aims to enhance the robustness, maintainability, and usability of epoch-related operations. The new Epoch class encapsulates complex logic for handling fractional parts, normalization, and arithmetic, while also providing a clear, well-documented API. This modernization improves type safety and prepares the codebase for more advanced features by integrating Epoch more deeply with the Molecule serialization framework.
Highlights
- Epoch Class Introduction: The
Epochdata structure has been transformed from a simple tuple[number, index, length]into a dedicated TypeScript class, providing a more object-oriented approach to epoch management. - New Utility Methods: The
Epochclass now includes comprehensive utility methods for creation (from,zero,one,oneCycle), comparison (compare,eq), arithmetic (add,sub), normalization (normalized), and conversion (toUnix). - Improved Documentation: Extensive TypeDoc-style comments have been added to the
Epochclass and its methods, detailing invariants, behaviors (e.g., positive denominator, fraction reduction via GCD, negative-index normalization), and usage. - Backward Compatibility & Deprecation: Existing tuple-based
epochFrom,epochFromHex, andepochToHexhelpers are deprecated, along with array-style accessors[0],[1],[2]on theEpochclass, encouraging migration to the new class methods. - Refactored DAO Claim Epoch Calculation: The
calcDaoClaimEpochfunction has been updated to leverage the newEpochclass, improving its logic and readability. ZeroConstant Usage: Replaced directnumFrom(0)calls with theZeroconstant for improved consistency and clarity in numerical operations.- Generic
toHexfor Entities: AtoHexmethod has been added to the basemol.Entity.Baseclass, allowing any Molecule entity to be easily converted to its hexadecimal representation.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
✅ Deploy Preview for appccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement by refactoring the Epoch type into a class, enhancing readability, type safety, and maintainability. The changes are well-structured, moving epoch-related logic to a new dedicated file and providing backward compatibility. The documentation is thorough and helpful. I've found one area for improvement in the compare method to ensure its correctness with very large numbers, which I've detailed in a specific comment. Overall, this is a high-quality contribution.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, refactoring Epoch from a tuple-based type into a dedicated class with comprehensive utility methods and documentation. This significantly enhances readability and maintainability. The related changes, such as using the Zero constant and updating the calcDaoClaimEpoch function, are also positive. I've identified one critical issue regarding a breaking change in the serialization format of the Epoch class and one minor typo in a code example. Addressing the serialization issue is crucial for backward compatibility.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
@Hanssen0 feel free to review 🤗 Phroi %16 |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, refactoring the Epoch type into a full-fledged class with comprehensive utility methods and excellent documentation. The new class-based approach significantly enhances readability and maintainability, especially evident in the simplification of calcDaoClaimEpoch. The deprecation of old helper functions and the introduction of backward-compatible properties are well-handled. I've found one area for improvement to make the new gcd utility function more robust.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, refactoring the Epoch type into a well-designed class with comprehensive documentation and utility methods. The approach to backward compatibility at the API level is commendable.
However, I've found a critical issue in the serialization of the new Epoch class. The implementation uses mol.struct, which changes the binary format from a packed u64 to a molecule struct. This breaks on-chain compatibility. I've left a detailed comment with a suggested fix to align the serialization with the expected CKB format.
Once this serialization issue is addressed, this will be an excellent contribution.
phroi
force-pushed
the
feat/epoch-utils
branch
from
a54e026 to
9738e04
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, refactoring the tuple-based Epoch into a full-featured class with comprehensive utility methods. The new Epoch class is well-designed with robust logic for construction, comparison, arithmetic, and normalization. The changes across the codebase to adopt this new class, such as removing the old helpers and using Zero for consistency, are well-executed. I've found one issue in a documentation example that should be addressed to avoid confusion for developers.
phroi
force-pushed
the
feat/epoch-utils
branch
from
9738e04 to
0997efb
Compare
phroi
force-pushed
the
feat/epoch-utils
branch
from
0997efb to
63aa8e4
Compare
|
@Hanssen0 I solved our previous naming issue and propagated changes, feel free to review once again!
About |
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@ckb-ccc/core": patch | |||
There was a problem hiding this comment.
Since this includes new APIs, it should be a minor version bump.
| * @param other - EpochLike to test equality against. | ||
| * @returns true if both epochs represent the same value. | ||
| */ | ||
| eq(other: EpochLike): boolean { |
There was a problem hiding this comment.
I think it will be great if we can also have gt & lt (ge & le).
| * | ||
| * This is a NervosDAO convenience constant. | ||
| */ | ||
| static oneCycle(): Epoch { |
There was a problem hiding this comment.
It will be better if we can have a more explicit name, like oneNervosDaoCycle nervosDaoCycle daoCycle oneDaoCycle
https://docs.rs/crate/ckb-gen-types/1.0.0-rc1/source/schemas/blockchain.mol#76 We should explicitly accept 64 bits only, to be consistent with CKB's molecule definition. I can accept a I might suggest that we add |
2 participants
Changes
Epochinto a class and added utility methodsfrom,zero/one/oneCycle, arithmeticadd/sub, comparisoncompare/eq, normalizationnormalized, andtoUnixconversion).epochFrom,epochFromHex,epochToHex.epochInMillisecondsandgcd.Closes #302
Love & Peace, Phroi %208