feat(core): transform Epoch into a class and add utilities
#314
Conversation
🦋 Changeset detectedLatest commit: d5be23b 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.
|
@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.
|
/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.
Hanssen0
left a comment
Hanssen0
left a comment
There was a problem hiding this comment.
For me, this implementation looks more like a mathematical aspect of a mixed number, rather than a CKB epoch. I suggest that we should separate these two concepts and make the Epoch stricter than this, to avoid "incorrect" index or length.
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
2 times, most recently
from
0997efb to
63aa8e4
Compare
|
@Hanssen0 I solved our previous naming issue and propagated changes, feel free to review once again!
About |
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 |
phroi
force-pushed
the
feat/epoch-utils
branch
2 times, most recently
from
6f59080 to
920cc53
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great initiative to refactor the Epoch representation into a dedicated class with utility methods, which significantly improves code clarity and usability. The changes are well-documented and the new methods for arithmetic and comparison are valuable additions. However, I've identified a few critical issues: one with the serialization/deserialization logic of the new Epoch class, and another with a reference error in calcDaoClaimEpoch. I've also included a suggestion for optimizing the add method. After addressing these points, this will be a very strong contribution.
phroi
force-pushed
the
feat/epoch-utils
branch
from
920cc53 to
c0faaf7
Compare
phroi
force-pushed
the
feat/epoch-utils
branch
from
7e5c6bb to
a82ee5c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the handling of epoch-related logic by refactoring Epoch from a tuple into a well-defined class. The new class provides a much cleaner and more robust API with methods for arithmetic, comparison, and normalization. The inclusion of comprehensive TypeDoc comments and maintaining backward compatibility are great additions. The related changes to use the new Epoch class and the Zero constant throughout the codebase are also well-executed and improve consistency. I have a few minor suggestions to further enhance the code's clarity and efficiency.
phroi
force-pushed
the
feat/epoch-utils
branch
from
a82ee5c to
ec70cb0
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, refactoring the Epoch representation from a tuple to a dedicated class. This enhances type safety and introduces a suite of utility methods for arithmetic, comparison, and normalization, making Epoch objects much easier and safer to work with. The changes are consistently applied throughout the codebase, and the new class is well-documented. I have a couple of suggestions to further improve correctness and documentation clarity.
phroi
force-pushed
the
feat/epoch-utils
branch
from
ec70cb0 to
5c05118
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, refactoring the Epoch representation from a simple type to a full-featured class. This significantly enhances code clarity, maintainability, and usability. The new class is well-documented with TypeDoc comments, and the inclusion of utility methods for arithmetic, comparison, and normalization is very helpful. The backward compatibility for old helpers and array-style properties is also a thoughtful touch.
I have a couple of suggestions to further improve the code by removing magic numbers, which will enhance readability and maintainability.
|
This should be the final form of this PR. I tried the code and it works, it also helps that the main decode/encode logic is very close to the previous one. @Hanssen0 do you have any feedback? Phroi %526 |
phroi
changed the title
Epoch into a class and add utility methodsEpoch into a class and add utilities
Hanssen0
left a comment
Hanssen0
left a comment
There was a problem hiding this comment.
I can see how the 0 denominator messes up everything. To be honest, I think the design is disgusting.
However, we have to deal with this anyway. I suggest mapping the denominator from 0 to 1 (only during calculation) to simplify our code. For example:
toValid(): Epoch {
if (this.denominator !== 0) {
return this;
}
return new Epoch(
this.integer,
this.numerator,
1,
);
}
compare(otherLike: EpochLike): 1 | 0 | -1 {
const other = Epoch.from(otherLike).toValid():
...
}
I considered it and it would lead to completely different results in edge cases:
One could then could look at I would say hell no, unless we are trying to encode |
You're right. I assume this can be fixed by mapping the numerator to 0 while mapping the denominator to 1 (by tweaking the |
That I'm too sleepy, I'll read again tomorrow and reply carefully. Night night 🤗 |
If we coerce also any
Then I'll proceed with the Epoch re-implementation of this and struct based codec |
I have a strong preference that this is the right way: |
OFC I agree!! I'd also like a word with those who chose Hopefully, if any issue with our right zero epoch ever arise, it just doesn't validate on L1 and it's easy to pinpoint, so it shouldn't cause too much trouble |
phroi
changed the title
Epoch into a class and add utilitiesEpoch into a class and add utilities
phroi
force-pushed
the
feat/epoch-utils
branch
from
5c05118 to
8708754
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Epoch class to replace the previous tuple-based representation, along with several utility functions and refactoring across the codebase to adopt this new class. The changes are well-documented and improve the type safety and usability of epoch-related logic. I've identified a couple of minor issues: a leftover console.log statement and a potential bug in the add method's logic. Additionally, the calcDaoClaimEpoch function has been significantly refactored, and while it appears correct, it would benefit from more comprehensive test cases to ensure its correctness under various scenarios.
phroi
force-pushed
the
feat/epoch-utils
branch
from
8708754 to
9685730
Compare
|
/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. This enhances encapsulation and makes the code more robust and easier to use. The new class is well-documented, and backward compatibility is thoughtfully maintained. I've identified one critical runtime error and one medium-severity robustness issue. Once these are addressed, the PR will be in excellent shape.
phroi
force-pushed
the
feat/epoch-utils
branch
from
9685730 to
d5be23b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant improvement, refactoring the Epoch representation from a simple type to a full-fledged class. This enhances type safety, encapsulation, and maintainability. The new Epoch class is well-documented and provides a rich set of utility methods for creation, normalization, comparison, and arithmetic, making it much easier to work with epoch values. The logic in calcDaoClaimEpoch is now more readable and leverages the new class effectively. The inclusion of backward-compatible deprecated helpers is a thoughtful touch for a smoother transition. Overall, this is a high-quality contribution. I have one minor suggestion to improve the robustness of a factory method.
|
@Hanssen0 I updated the implementation following your feedback, feel free to review it once again 🤗 Phroi %354 P.S.: watch out for the new npm hack |
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