feat(core): add CellAny
#262
Conversation
🦋 Changeset detectedLatest commit: 71659a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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 appccc 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 liveccc 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 @Hanssen0, 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 introduces a new CellAny class within the core module, designed to serve as a unified representation for both on-chain and off-chain cells. This change aims to reduce code duplication by allowing the existing Cell class to extend CellAny, thereby centralizing common cell-related functionalities.
Highlights
- New Base Class: A new CellAny class has been introduced to abstract common properties and methods for both on-chain and off-chain cells.
- Inheritance: The existing Cell class now extends CellAny, inheriting its properties and methods, which streamlines the codebase.
- Code Refactoring: Shared methods such as capacityFree, occupiedSize, and isNervosDao have been moved from the Cell class to the new CellAny base class, promoting reusability and reducing duplication.
- Constructor and Type Updates: The Cell class constructor has been updated to properly call the CellAny super constructor, and the CellLike type definition has been adjusted to align with the new CellAnyLike structure.
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. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new CellAny class to represent both on-chain and off-chain cells, which is a great refactoring to reduce code duplication. The existing Cell class now extends CellAny, inheriting common functionality. The changes are well-structured and improve the overall design. I've added a couple of suggestions to further improve code conciseness and maintainability by reusing existing utilities and getters.
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Hanssen0
force-pushed
the
feat/cell-any
branch
from
c86b4ed to
8e435d9
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new CellAny class to represent both on-chain and off-chain cells, which is a great step towards reducing code duplication. The existing Cell class for on-chain cells now correctly extends CellAny, and methods like occupiedSize, capacityFree, and isNervosDao have been moved to the base class.
My main feedback is regarding property shadowing in the Cell constructor, which could be improved for better maintainability. Overall, the changes are well-structured and improve the codebase.
Hanssen0
force-pushed
the
feat/cell-any
branch
3 times, most recently
from
8bd6c41 to
abc44f1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces CellAny to represent both on-chain and off-chain cells, which is a great architectural improvement. The existing Cell class now extends CellAny, effectively reducing code duplication and clarifying the distinction between on-chain and potentially off-chain cells. The changes are well-implemented. My feedback includes a couple of minor suggestions to improve code maintainability by avoiding repeated expressions.
Hanssen0
force-pushed
the
feat/cell-any
branch
3 times, most recently
from
8cbbe72 to
3cb1dad
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new CellAny class to represent both on-chain and off-chain cells, which is a significant improvement for code clarity and reducing duplication. The refactoring of the existing Cell class to extend CellAny and specifically represent on-chain cells is well-executed. The related changes to the Transaction class to adopt CellAny are consistent and enhance the overall API. My review includes a couple of suggestions to further improve the readability and maintainability of the new code.
Hanssen0
force-pushed
the
feat/cell-any
branch
from
3cb1dad to
00513ee
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new CellAny class to represent both on-chain and off-chain cells, which is a great step towards reducing code duplication and clarifying the distinction that was previously blurred by the Cell class. The refactoring of Cell to extend CellAny is well-executed, and the related API simplifications in CellOutput and Transaction make the code cleaner and more intuitive to use. The changes are solid and improve the overall design. I have one suggestion to improve the robustness of a function call.
It's definitely a mistake to name `CellOnChain` `Cell`, but there is nothing we can do with that right now. To avoid more duplicate code, `CellAny` was added to represent a cell that's on-chain or off-chain.
Hanssen0
force-pushed
the
feat/cell-any
branch
from
00513ee to
71659a2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new CellAny class to represent both on-chain and off-chain cells, which is a great refactoring. The existing Cell class, which represents on-chain cells, now correctly inherits from CellAny. This change effectively reduces code duplication by moving common properties and methods like occupiedSize, capacityFree, and isNervosDao to the new base class. The related types and APIs, such as Transaction.getOutput and Transaction.addOutput, have been updated to use this new abstraction, making them cleaner and more consistent. The changes are well-implemented across the core and ssri packages. Overall, this is a high-quality contribution that improves the structure and maintainability of the codebase.
|
Finally, we make our overlord satisfied. |
|
I wonder if there is a better name for CellAny |
|
Also it would be nice a method for promotion to ccc.Cell by checking Client |
This is the best I can get. Any suggestions are welcome.
I think we can't get the |
That's correct, I realized one minute too late 🤣 |
|
By the way, humor me here, a Cell(Any) has always an outpoint |
|
Just the hash may change as the original transaction is modified |
|
Not if we use this CellAny as template to add output cells, ok |
|
I stand by my comment: all cells (input and output) in the context of a Transaction are ccc.Cell Just the hash of outputs may change as the Transaction is updated |
You noticed me, it does. This point might be helpful when determining an outPoint before sending a transaction. |
|
So we may actually not need ccc.CellAny |
I would say it's still needed. Putting off chain outpoints into |
|
You probably are right, as usual. Just let's remember that even chained transactions output should be considered invalid until they are confirmed on-chain and enough time has passed, so no re-org can happen. Otherwise even those can be invalid, say for example that an input is consumed by a competing transaction, then the outpoints become invalid
This seems realistic, just could you make a realistic example? |
Do you fancy any of these? I asked help to GPT-o4 mini 😁
|
|
Yea I also ask Gemini... didn't get a better name. Other names like |
|
Ok, the concept is sound, the code looks in order and well thought. Just one additional question: would it make sense to add an additional parameter? Something along the lines of
|
Considering that |
|
Yeah, this may be more of a concept related to the code consuming these cells like Looks good to me, feel free to merge!! |
2 participants
It's definitely a mistake to name
CellOnChainCell, but there is nothing we can do with that right now. To avoid more duplicate code,CellAnywas added to represent a cell that's on-chain or off-chain.