Skip to content

The Refactor#735

Merged
yeasy merged 118 commits intohyperledger-cello:mainfrom
dodo920306:task/the-refactoring
Mar 28, 2026
Merged

The Refactor#735
yeasy merged 118 commits intohyperledger-cello:mainfrom
dodo920306:task/the-refactoring

Conversation

@dodo920306
Copy link
Copy Markdown
Contributor

Let's face it: the current state of this project’s code is not so good.

Contributions over time have lacked consistency, leading to duplicated implementations and a fragmented structure that makes maintenance and development difficult.

The combination of Python, Django, and object-oriented design has introduced unnecessary complexity, and without long-term technical stewardship, the project has become hard to extend.

Another pressing issue is the number of long-standing bugs. For a project that has been in existence for over six years, there remain issues that ideally should have been addressed very early on. Many of these are not superficial problems that tools or automated testing could easily detect, but deeper logical flaws that only become evident when the affected code paths are used.

Additionally, it seems that the project has not fully embraced the design principles of Django REST Framework (DRF). For example, serializers are often used only for traffic handling, while much of the core business logic resides in views.py, which goes against DRF’s intended design practices.

To summary, a refactor is inevitable, and to make sure I’m not just another person who criticizes without acting, I’ll do it myself.

@dodo920306 dodo920306 closed this Aug 29, 2025
@dodo920306 dodo920306 deleted the task/the-refactoring branch August 29, 2025 11:22
@dodo920306 dodo920306 restored the task/the-refactoring branch August 29, 2025 11:22
@dodo920306 dodo920306 reopened this Aug 29, 2025
@dodo920306 dodo920306 force-pushed the task/the-refactoring branch 2 times, most recently from 4a3123e to 583629c Compare October 6, 2025 07:56
@dodo920306
Copy link
Copy Markdown
Contributor Author

Philosophy

One key principle behind this PR is that unused code should be removed from the codebase.

If a piece of code becomes relevant again in the future, it can always be recovered from Git history. That’s precisely what version control is for — preserving past implementations without cluttering the present codebase.

API Engine

This is where most of the refactoring takes place, so let’s start here.

"Agents Are Actually Redundant"

This may sound controversial at first, but I genuinely believe that once we decided to adopt Docker Swarm (which we must, given blockchain's network accessibility requirements), the concept of "agents" in the Hyperledger Cello architecture became unnecessary.

The Hyperledger Fabric binaries we rely on are already well-equipped to operate nodes (peers and orderers) remotely — for example, using the familiar --peerAddresses and --orderer-address flags. Moreover, Docker Swarm (and Kubernetes in the future) is specifically designed to manage nodes remotely.

This leads to a simple conclusion: since both the infrastructure layer (Docker/Kubernetes) and the application layer (Fabric) already provide remote control capabilities, there's no need to implement another redundant remote-control layer ourselves. We can directly leverage the built-in functionality of these tools.

As a result, the agent concept will be almost entirely removed after this change.

Structure

Another major improvement is the API Engine structure.
Instead of cramming all modules into a single application called app, I now separate them into distinct Django applications — as Django was originally designed to handle — since they represent different conceptual layers.

Views-Serializers-Services-Models

If a module needs some logic implementations, the module should be divided into 4 files (at least).

  1. Views -- Handle requests and responses, delegating work to appropriate serializers.
  2. Serializers -- Validate requests and responses, and perform necessary logic by calling services.
  3. Services -- Implement business logic by manipulating ORM models.
  4. Models -- Define ORM structures themselves.

Previous, most logic was tightly packed inside views.py, with only data entities defined in models.py. This made the code difficult to maintain.
By splitting modules into these four layers, we achieve much clearer separation of concerns and improved maintainability.

Code Style

As mentioned in the Philosophy section, all legacy code unrelated to current functionality has been removed. Additionally, redundant or incorrect implementations have been cleaned up. Here are a few examples:

  1. settings.py.examplesettings.py
    Since Python can naturally read environment variables, there's no need for envsubst.
    This change also allows static analysis tools and IDEs (such as PyCharm, which I use heavily) to recognize the file as a standard Python module.
    A hardcoded API key was also removed — it should never have been there in the first place.
  2. Improper validation checks
    There were many cases like:
    ...
    if serializer.is_valid(raise_exception=True):
    ...
    This smells terrible as hell because is_valid(raise_exception=True) already raises an exception automatically when validation fails — no conditional check is needed.
  3. Editable primary keys
    Instances like:
    ...
    class XXX(models.Model):
        id = models.UUIDField(
            primary_key=True,
            help_text="ID of organization",
            default=make_uuid,
            editable=True,
    )
    ...
    also smell terrible because they're conceptually wrong — a primary key should never be editable.
  4. Enum improvements
    All enum names are now uppercase, reflecting their constant nature.
    Enums related to models now use Django's native models.TextChoices instead of plain Python enums, providing a much better developer experience (since they can be used as values directly).

@dodo920306 dodo920306 marked this pull request as ready for review October 10, 2025 14:04
@dodo920306
Copy link
Copy Markdown
Contributor Author

Delivery

  1. The Docker files now are moved into api-engine and dashboard so that in docker-compose files they can be built with context.

Side Effect

  1. The original APIs aren't fully re-implemented since I believe that the deletion logic didn't fully include deletion of crypto materials, which made it fail when it comes to re-creating anything. They will be added back once they're ready. Currently, the implemented APIs are only the GET and POST ones.
  2. Some CI steps are skipped because the new docker file locations and the bad compatibility with the old APIs causing the newman tests to fail.

@yeasy
Copy link
Copy Markdown
Contributor

yeasy commented Nov 20, 2025

@YoungHypo suggest we create a new branch for this refactor work.

@dodo920306 dodo920306 changed the base branch from main to lab/kirin-refactor November 21, 2025 07:05
@dodo920306
Copy link
Copy Markdown
Contributor Author

dodo920306 commented Nov 21, 2025

@YoungHypo suggest we create a new branch for this refactor work.

I just realized that I myself can also create a new branch in this repo. Therefore, I created a new branch called lab/kirin-refactor as a new target for this PR.

Feel free to suggest another branch name.

@dodo920306
Copy link
Copy Markdown
Contributor Author

dodo920306 commented Jan 3, 2026

With the first interaction with a chaincode via Hyperledger Cello done on my PC, I hereby proudly announce that now Hyperledger Cello API Engine has the very basic ability to interact with chaincodes after a1f9e92 🥳!

This definitely took a lot longer than expected, and the functionality is definitely not mature enough now ---- it is currently inaccessible via dashboard.

螢幕快照 2026-01-03 20-30-06

@yeasy

@dodo920306
Copy link
Copy Markdown
Contributor Author

dodo920306 commented Jan 3, 2026

Update for this PR:

After several meetings and discussions, I officially retract some of my comments in #735 (comment) and acknowledge that agents do indeed have their necessity.

However, I also raised my concerns during the meetings — namely, that the logic for interacting with blockchain nodes on the main branch is almost entirely concentrated in the api-engine. The agent, acting as a proxy within each organization's cluster, still relies on the api-engine — which exists independently of all clusters — to perform operations. This greatly reduces the necessity of installing agents in each cluster, and also calls for a rethinking of the network layer design.

Subsequently, @yeasy proposed moving all current blockchain interaction logic into the agent, allowing the agent to handle all operations. This also aligns with the original intention of Hyperledger Cello: to move blockchain-specific operations into agents, enabling users to support various blockchain projects simply by swapping agents. Fortunately, after this refactoring, user request handling is decoupled from business logic to the maximum extent (i.e., a view-service layered design). This makes it relatively easier to extract service logic and insert it into a standalone agent project compared to the main branch, so implementing a new agent might be easier than explained during the meetings.

In any case, reintegrating the decoupled agent will be the next and final stage of this refactoring. Hopefully, this process will go more smoothly this time.

@dodo920306 dodo920306 force-pushed the task/the-refactoring branch from edeabb0 to eb936ad Compare January 4, 2026 15:47
@yeasy
Copy link
Copy Markdown
Contributor

yeasy commented Jan 13, 2026

And as we discussed, the new model can be pushed a a new branch such as refactor.

@dodo920306
Copy link
Copy Markdown
Contributor Author

And as we discussed, the new model can be pushed a a new branch such as refactor.

Yeah. So I did.

@dodo920306 dodo920306 marked this pull request as draft January 17, 2026 07:03
@dodo920306 dodo920306 changed the base branch from lab/kirin-refactor to main February 21, 2026 00:40
@dodo920306 dodo920306 marked this pull request as ready for review February 21, 2026 01:13
@dodo920306
Copy link
Copy Markdown
Contributor Author

@yeasy Here is the latest update. I've reintroduced the agents as discussed, and all Hyperledger Fabric operations have now been consolidated within them.

This change inevitably increased the scope of the PR. Ideally, some of this restructuring could have been split into a follow-up PR, but given the current context, I've consolidated everything here to keep the refactor coherent.

At this stage, I believe the branch is in a mergeable state. While there is still room for improvement, those refinements can be addressed incrementally in subsequent PRs.

Since this branch is already significantly ahead of main (118 commits), I think it would be healthier for the project to bring main up to date with the refactor, rather than continuing to extend a long-running branch. Keeping branches aligned will make future development and review cycles much smoother.

If reviewing the full diff is challenging due to its size, I suggest focusing on the resulting architecture and overall behavior. I'm of course happy to clarify any specific concerns.

Given the above, I've updated the target branch to main. If you feel there's a strong reason to follow a different merge strategy, I'm open to discussing it.

Overall, I believe this PR is ready to move forward.

@yeasy
Copy link
Copy Markdown
Contributor

yeasy commented Feb 21, 2026

It will be better to have stable code before merging into main.
We need more test cases. can you help enhance the testing?

@dodo920306
Copy link
Copy Markdown
Contributor Author

It will be better to have stable code before merging into main. We need more test cases. can you help enhance the testing?

I can try, but I’d like to point out that doing so within this PR will further increase its scope and complexity.

@yeasy
Copy link
Copy Markdown
Contributor

yeasy commented Feb 23, 2026

Another approach is that checkout a new v1-stable branch and merge your refactor into main.
How do you think?
@dodo920306

@dodo920306
Copy link
Copy Markdown
Contributor Author

Another approach is that checkout a new v1-stable branch and merge your refactor into main. How do you think? @dodo920306

That's actaully a good idea! We can check out a release/1.2 branch from f9d56cf (tag v1.2.0).

@yeasy
Copy link
Copy Markdown
Contributor

yeasy commented Feb 24, 2026

Another approach is that checkout a new v1-stable branch and merge your refactor into main. How do you think? @dodo920306

That's actaully a good idea! We can check out a release/1.2 branch from f9d56cf (tag v1.2.0).

@YoungHypo how do you think?

@YoungHypo
Copy link
Copy Markdown
Member

Another approach is that checkout a new v1-stable branch and merge your refactor into main. How do you think? @dodo920306

That's actaully a good idea! We can check out a release/1.2 branch from f9d56cf (tag v1.2.0).

@YoungHypo how do you think?

LGTM.

@YoungHypo YoungHypo requested a review from Copilot February 28, 2026 02:24
@yeasy yeasy review requested due to automatic review settings February 28, 2026 02:27
Signed-off-by: dodo920306 <dodo920306@gmail.com>
These values can be retrieved from files dynamically
whenever they're wanted. They shouldn't be stored
as DB columns.

Signed-off-by: dodo920306 <dodo920306@gmail.com>
They're not necessary.

Signed-off-by: dodo920306 <dodo920306@gmail.com>
Node types are parsed as strings rather than enums.

Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Use 'w' instead of 'r+' to overwrite files instead of
just appending contents.

Signed-off-by: dodo920306 <dodo920306@gmail.com>
Package MSP and TLS crypto materials into their
own directories respectively.

Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Save TLS certificates in the DB so that they can
be accessed by other organizations in the future.

Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
1. Remove Chaincode peers input from the form
2. Sync Node status with status returned by the Docker Python API
3. Remove Channel peers/orders input from the form

Signed-off-by: dodo920306 <dodo920306@gmail.com>
Response chaincode status (CREATED, INSTALLED, APPROVED,
COMMITTED) for the chaincode page.

Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Signed-off-by: dodo920306 <dodo920306@gmail.com>
Replace `make` with `docker compose` commands with
`docker-compose.dev.yaml` as the config file.

Signed-off-by: dodo920306 <dodo920306@gmail.com>
In the Hyperledger Fabric docker agent, I want to igore files starting
with '.' like `.venv` for the python virtual environment for personal
development. Thus, I added `.*` in the `.dockerignore`. However,
this causes a warning when building the docker image because for
`COPY . .` because the current path `.` is also included in `.*`.
Therefore, `!.` is added to the dockerfile to suppress the warning.

Signed-off-by: dodo920306 <dodo920306@gmail.com>
The Hyperledger Fabric agent in the CI pipeline aren't assigned with
the network and the docker socket volume, so it becomes inaccessible.

Signed-off-by: dodo920306 <dodo920306@gmail.com>
@dodo920306 dodo920306 force-pushed the task/the-refactoring branch from efe7764 to 1dfba83 Compare March 28, 2026 01:59
@dodo920306
Copy link
Copy Markdown
Contributor Author

@yeasy I believe the commits are verified now.

@yeasy yeasy merged commit ceebb47 into hyperledger-cello:main Mar 28, 2026
3 checks passed
@yeasy
Copy link
Copy Markdown
Contributor

yeasy commented Mar 28, 2026

@yeasy I believe the commits are verified now.

Done, thanks!

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