update clearnode structure #473
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @dimast-x, 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 represents a substantial refactoring of the clearnode's internal architecture. The primary focus is on enhancing modularity and maintainability by reorganizing API components into distinct packages and standardizing core utilities. A fundamental change involves a comprehensive update to the database schema, introducing new data models for states and ledger transactions while simplifying existing channel structures. These changes aim to streamline the system, improve data integrity with deterministic IDs, and lay a more robust foundation for future development, even if it means deprecating certain functionalities like channel resizing and user tagging in the process. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring of the Clearnode codebase, primarily by reorganizing files into new api, custody, db, log, memory, prometheus, and sign packages to improve modularity and separation of concerns. Key changes include the removal of the AuthManager and MessageCache from the RPC router, the elimination of the resize_channel RPC method and its associated logic, and the deprecation of user tags. The Channel data model has been updated to remove RawAmount, Participant, and Adjudicator fields, replacing them with UserWallet, Type, and BlockchainID, and introducing OnChainStateVersion. The LedgerTransaction model now uses string IDs and includes SenderNewStateID and ReceiverNewStateID fields. Several critical issues were identified in the review: the ensureWalletHasAllAllocationsEmpty validation check was temporarily bypassed, the custodyAbi initialization was commented out, and the logic for responding to on-chain challenges was removed, creating potential security vulnerabilities. Additionally, the TransactionResponse struct still contains FromAccountTag and ToAccountTag fields despite the user tag feature being removed, and a TODO comment suggesting merging service and handler layers was advised against due to best practices in separation of concerns.
| func ensureWalletHasAllAllocationsEmpty(tx *gorm.DB, wallet string) error { | ||
| channelAmountSum, err := GetChannelAmountSumByWallet(tx, wallet) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !channelAmountSum.Sum.IsZero() { | ||
| return RPCErrorf("operation denied: non-zero allocation in %d channel(s) detected owned by wallet %s", channelAmountSum.Count, wallet) | ||
| } | ||
| // TODO: Implement balance checking using the new State model | ||
| // This function needs to be redesigned to check balances from the State model | ||
| // instead of the Channel model, as balances are now tracked per-state. | ||
| // For now, we return nil to allow the code to compile. | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The function ensureWalletHasAllAllocationsEmpty has been stubbed out and now always returns nil. The TODO indicates this is temporary, but this bypasses a critical validation check that prevents users from creating new sessions or depositing funds while they have allocations in other open channels. This check must be re-implemented with the new data model to prevent potential fund locking or inconsistent states.
| // func init() { | ||
| // var err error | ||
| // custodyAbi, err = nitrolite.CustodyMetaData.GetAbi() | ||
| // if err != nil { | ||
| // panic(err) | ||
| // } | ||
| // } | ||
|
|
There was a problem hiding this comment.
The init() function that initialized the global custodyAbi variable has been commented out. This variable is used in custody.go to parse blockchain events. Without initialization, this will cause a nil pointer dereference at runtime when HandleBlockChainEvent is called, effectively breaking all on-chain event processing. The ABI must be initialized correctly in the custody package.
| // challengedVersion := ev.State.Version.Uint64() | ||
| // localVersion := channel.State.Version | ||
|
|
||
| logger.Warn("challenge detected", "challengedVersion", challengedVersion, "localVersion", localVersion, "channelId", channelID) | ||
| // logger.Warn("challenge detected", "challengedVersion", challengedVersion, "localVersion", localVersion, "channelId", channelID) | ||
|
|
||
| if challengedVersion < localVersion { | ||
| if channel.UserStateSignature != nil && channel.ServerStateSignature != nil { | ||
| if err := CreateCheckpoint(tx, channelID, c.chainID, channel.State, *channel.UserStateSignature, *channel.ServerStateSignature); err != nil { | ||
| logger.Error("failed to create checkpoint", "error", err) | ||
| } else { | ||
| logger.Info("created checkpoint action", "channelId", channelID, "localVersion", localVersion, "challengedVersion", challengedVersion) | ||
| } | ||
| } else { | ||
| logger.Warn("detected local state in db without signatures that is newer than a challenged one", "channelId", channelID) | ||
| } | ||
| } | ||
| // if challengedVersion < localVersion { | ||
| // if channel.UserStateSignature != nil && channel.ServerStateSignature != nil { | ||
| // if err := CreateCheckpoint(tx, channelID, c.chainID, channel.State, *channel.UserStateSignature, *channel.ServerStateSignature); err != nil { | ||
| // logger.Error("failed to create checkpoint", "error", err) | ||
| // } else { | ||
| // logger.Info("created checkpoint action", "channelId", channelID, "localVersion", localVersion, "challengedVersion", challengedVersion) | ||
| // } | ||
| // } else { | ||
| // logger.Warn("detected local state in db without signatures that is newer than a challenged one", "channelId", channelID) | ||
| // } | ||
| // } | ||
| channel.Status = ChannelStatusChallenged |
There was a problem hiding this comment.
The logic for responding to an on-chain challenge has been commented out. In a state channel system, it's critical to be able to respond to a challenge with a more recent state to prevent theft of funds. By removing this logic, the node is left vulnerable to an adversary publishing an old state on-chain. This is a critical security vulnerability and must be fixed.
| @@ -0,0 +1,125 @@ | |||
| package app_session_api | |||
|
|
|||
| // TODO: merge service and handlers | |||
There was a problem hiding this comment.
The TODO comment suggests merging the service and handlers. I would recommend against this. Keeping handlers and services separate is a good practice for separation of concerns. Handlers should be responsible for HTTP/RPC-level concerns (parsing requests, formatting responses), while services contain the core business logic. This separation improves testability, maintainability, and allows the business logic to be reused with different transport layers.
|
|
||
| import "github.com/ethereum/go-ethereum/common" | ||
|
|
||
| // TODO: merge service and handlers |
There was a problem hiding this comment.
The TODO comment suggests merging the service and handlers. I would recommend against this. Keeping handlers and services separate is a good practice for separation of concerns. Handlers should be responsible for HTTP/RPC-level concerns (parsing requests, formatting responses), while services contain the core business logic. This separation improves testability, maintainability, and allows the business logic to be reused with different transport layers.
| type TransactionResponse struct { | ||
| Id uint `json:"id"` | ||
| TxType string `json:"tx_type"` | ||
| FromAccount string `json:"from_account"` | ||
| FromAccountTag string `json:"from_account_tag,omitempty"` // Optional tag for the source account | ||
| ToAccount string `json:"to_account"` | ||
| ToAccountTag string `json:"to_account_tag,omitempty"` // Optional tag for the destination account | ||
| Asset string `json:"asset"` | ||
| Amount decimal.Decimal `json:"amount"` | ||
| CreatedAt time.Time `json:"created_at"` | ||
| Id string `json:"id"` | ||
| TxType string `json:"tx_type"` | ||
| FromAccount string `json:"from_account"` | ||
| FromAccountTag string `json:"from_account_tag,omitempty"` // Optional tag for the source account | ||
| ToAccount string `json:"to_account"` | ||
| ToAccountTag string `json:"to_account_tag,omitempty"` // Optional tag for the destination account | ||
| Asset string `json:"asset"` | ||
| Amount decimal.Decimal `json:"amount"` | ||
| SenderNewStateID *string `json:"sender_new_state_id,omitempty"` | ||
| ReceiverNewStateID *string `json:"receiver_new_state_id,omitempty"` | ||
| CreatedAt time.Time `json:"created_at"` | ||
| } |
There was a problem hiding this comment.
The TransactionResponse struct still contains FromAccountTag and ToAccountTag fields. However, the user tag feature seems to have been removed in this pull request (e.g., tag.go deleted, HandleGetUserTag removed). For consistency and to avoid confusion, these fields should be removed from the TransactionResponse struct.
No description provided.