Feat/v2 f3 #310
Feat/v2 f3 #310
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request introduces support for Lotus v2 APIs to leverage F3 finality, which is a significant and valuable feature. The changes are extensive, touching the main application logic and all Rosetta services to handle v1 and v2 API clients, and to process finality tags from requests. New helper functions and tests for the v2 API logic have been added, which is great.
My review has identified a few key areas for improvement. There are a couple of places where the new v2 API logic is not fully utilized, with calls still being made to the v1 API, which could defeat the purpose of the finality feature. I've also noted some opportunities to improve testing, particularly by adding coverage for the new finality-aware code paths in the service-level tests and by refactoring a test that duplicates implementation logic. Additionally, there are some minor cleanup opportunities in the Makefile and error handling.
Overall, the changes are well-structured. Addressing the feedback will help ensure the new features are robust, correct, and well-tested.
README.md
Outdated
| } | ||
| ``` | ||
|
|
||
| **Key Behavior**: With finality tags, the proxy returns `max(requested_height, finality_height)`. |
There was a problem hiding this comment.
This behavior depends on this flag right?
FORCE_SAFE_F3_FINALITY
If it is not enable, it is max what we return. When enabled, if the client as for height 1000 and tag is provided, but height returned by the provided tag is 998, we reject and give an error?
There was a problem hiding this comment.
This logic is reworked in latest design document. The FORCE_SAFE_F3_FINALITY is removed in favor of anchor mode. With anchor mode enabled, the requested height is always returned with disabled the max(requested, finality) is returned
There was a problem hiding this comment.
No default finality can be forced only specified in request
main.go
Outdated
| if strings.HasSuffix(addr, "/rpc") { | ||
| // New format: base URL ends with /rpc | ||
| // Append /v1 and /v2 to create specific endpoints | ||
| v1Addr = addr + "/v1" | ||
| v2Addr = addr + "/v2" | ||
| srv.Logger.Infof("Using base /rpc endpoint - V1: %s, V2: %s", v1Addr, v2Addr) | ||
| } else if strings.Contains(addr, "/rpc/v1") { | ||
| // Legacy format: already has /rpc/v1 | ||
| v1Addr = addr | ||
| v2Addr = strings.Replace(addr, "/rpc/v1", "/rpc/v2", 1) | ||
| srv.Logger.Infof("Using legacy /rpc/v1 endpoint - V1: %s, V2: %s", v1Addr, v2Addr) | ||
| } else if strings.Contains(addr, "/rpc/v2") { | ||
| // If someone provides v2 endpoint directly, derive v1 from it | ||
| v1Addr = strings.Replace(addr, "/rpc/v2", "/rpc/v1", 1) | ||
| v2Addr = addr | ||
| srv.Logger.Infof("Using /rpc/v2 endpoint - V1: %s, V2: %s", v1Addr, v2Addr) | ||
| } else { | ||
| // Unrecognized format - return error | ||
| return nil, nil, nil, fmt.Errorf("unrecognized RPC endpoint format: %s. Expected format ending with /rpc, /rpc/v1, or /rpc/v2", addr) | ||
| } |
There was a problem hiding this comment.
can we extract this into a utils function?
There was a problem hiding this comment.
done! it's now in helper.go
main.go
Outdated
| v2Client, v2Closer, err := client.NewFullNodeRPCV2(context.Background(), v2Addr, headers) | ||
| if err != nil { | ||
| srv.Logger.Warnf("V2 APIs enabled but failed to create V2 client: %v. Will use V1 only", err) | ||
| return v1Client, nil, v1Closer, nil | ||
| } |
There was a problem hiding this comment.
If v2 is required, but fail to connect, I think we are hiding the error, as we are not returning it
main.go
Outdated
| rosettaLib *rosettaFilecoinLib.RosettaConstructionFilecoin, | ||
| ) http.Handler { | ||
| accountAPIService := srv.NewAccountAPIService(network, &api, rosettaLib) | ||
| accountAPIService := srv.NewAccountAPIService(network, &v1API, v2API, rosettaLib) |
There was a problem hiding this comment.
Any reason why one is a reference (v1), while the other is not (v2)
There was a problem hiding this comment.
Fixed the stuct def, not i's similar
There was a problem hiding this comment.
This mempool endpoint is meant to retrieve pending mempool txs, which indicates they are not really on any tipset yet. Asking for mempool txs with a tipset key different to latest may be inconsistent. We should check what that value is meant for on lotus node. Maybe we don't really need v2 features on it (just v1, or v2 with latest)
There was a problem hiding this comment.
I agree, we don't need v2 on mempool.
rosetta/services/block.go
Outdated
| if finalityTag != "" { | ||
| // Get the finality-based tipset | ||
| finalityTipSet, err := ChainGetTipSetWithFallback(ctx, s.v1Node, s.v2Node, finalityTag) | ||
| if err != nil { | ||
| return nil, BuildError(ErrUnableToGetTipset, err, true) | ||
| } | ||
| finalityHeight := int64(finalityTipSet.Height()) | ||
|
|
||
| // Return the maximum between requested and finality-based height | ||
| if requestedHeight >= finalityHeight { | ||
| // Requested block is already at or beyond finality height, return it | ||
| impl := func() { | ||
| tipSet, err = s.v1Node.ChainGetTipSetByHeight(ctx, abi.ChainEpoch(requestedHeight), filTypes.EmptyTSK) | ||
| } | ||
| errTimeOut := tools.WrapWithTimeout(impl, LotusCallTimeOut) | ||
| if errTimeOut != nil { | ||
| return nil, ErrLotusCallTimedOut | ||
| } | ||
| if err != nil { | ||
| return nil, BuildError(ErrUnableToGetTipset, err, true) | ||
| } | ||
| } else { | ||
| // Requested block is before finality, return the finality-based block | ||
| tipSet = finalityTipSet | ||
| requestedHeight = finalityHeight | ||
| } | ||
| } else { | ||
| // No finality tag - return the requested block | ||
| impl := func() { | ||
| tipSet, err = s.v1Node.ChainGetTipSetByHeight(ctx, abi.ChainEpoch(requestedHeight), filTypes.EmptyTSK) | ||
| } | ||
| errTimeOut := tools.WrapWithTimeout(impl, LotusCallTimeOut) | ||
| if errTimeOut != nil { | ||
| return nil, ErrLotusCallTimedOut | ||
| } | ||
| if err != nil { | ||
| return nil, BuildError(ErrUnableToGetTipset, err, true) | ||
| } |
There was a problem hiding this comment.
What if we move this into a different utils function, so that it is easier to review?
There was a problem hiding this comment.
It's now part of single height resolution function in v2Helper.go
|
/gemini review |
|
/gemini summary |
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality by adding support for the Lotus v2 API and F3 finality. The changes are extensive, touching the main application entrypoint, all Rosetta services, and adding new helper files for the finality logic. While the overall implementation is solid and includes some nice improvements like better error handling in main.go, I've identified a few issues. These include a critical bug that could lead to a panic, a high-severity issue causing data inconsistency in the account balance endpoint, and some medium-severity concerns regarding code robustness and maintainability. My detailed comments below provide explanations and suggestions for addressing these points.
Summary of ChangesThis pull request significantly upgrades the Rosetta Filecoin Proxy by integrating the Lotus V2 API, which introduces crucial finality features (F3). The changes involve extensive refactoring of core services to support dual API versions, enabling more precise and configurable data retrieval for blocks and accounts. Additionally, it includes important dependency updates and enhancements to the development and testing infrastructure. Highlights
Changelog
Ignored Files
Activity
|
|
@lawRathod for CI to work please start the calibration-next node on dev |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for Lotus v2 APIs and F3 finality, which is a significant and valuable feature. The implementation is well-structured, with the new finality logic correctly centralized in rosetta/services/v2_helpers.go. The changes also include several good refactorings, such as handling previously ignored errors and cleaning up API client creation.
My review focuses on ensuring the correctness and consistency of the new API extensions. I've identified a few areas for improvement:
- There's an issue with how sub-network identifiers are exposed in the
/network/listendpoint, which could affect client compatibility. - A potential correctness bug in the account balance calculation due to an incorrect fallback logic.
- A minor opportunity to reduce code duplication in the main function.
Overall, this is a solid contribution. Addressing these points will further improve the robustness and clarity of the implementation.
3 participants
🔗 zboto Link