cello icon indicating copy to clipboard operation
cello copied to clipboard

The Refactor

Open dodo920306 opened this issue 4 months ago • 4 comments

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 avatar Aug 29 '25 09:08 dodo920306

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 avatar Oct 10 '25 14:10 dodo920306

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.

dodo920306 avatar Oct 11 '25 00:10 dodo920306

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

yeasy avatar Nov 20 '25 06:11 yeasy

@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 avatar Nov 21 '25 07:11 dodo920306

The changes here have been split into 9 PRs:

  1. #741
  2. #742
  3. #743
  4. #744
  5. #745
  6. #746
  7. #747
  8. #748
  9. #749

dodo920306 avatar Dec 14 '25 16:12 dodo920306