PLT-7701, PLT-7702, PLT-7703, PLT-8427: Implement remaining 1-1 endpoints #128
Conversation
WalkthroughThe codebase has been updated to enhance contract source management and processing within a REST client context. New functions for retrieving contract sources by ID, adjacency, and closure have been added, along with type guards for response validation. The client interface now includes methods for these functions, and the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files selected for processing (3)
- packages/runtime/client/rest/src/contract/endpoints/sources.ts (2 hunks)
- packages/runtime/client/rest/src/contract/next/endpoint.ts (3 hunks)
- packages/runtime/client/rest/src/index.ts (10 hunks)
Additional comments: 7
packages/runtime/client/rest/src/contract/endpoints/sources.ts (3)
44-52: The
GetContractBySourceIdRequestandGetContractBySourceIdResponseinterfaces are well-defined, and the type guardGetContractBySourceIdResponseGuardis correctly usingG.Contractfor validation.75-103: The
GetContractSourceAdjacencyRequestandGetContractSourceAdjacencyResponseinterfaces are correctly defined, and the type guardGetContractSourceAdjacencyResponseGuardis properly implemented usingt.array(G.BuiltinByteString).105-114: The
GetContractSourceClosureRequestandGetContractSourceClosureResponseinterfaces are correctly defined, and the type guardGetContractSourceClosureResponseGuardis properly implemented usingt.array(G.BuiltinByteString).packages/runtime/client/rest/src/contract/next/endpoint.ts (3)
1-6: The imports for
io-tsandlanguage-core-v1/guardsare correctly added to support the new type definitions and runtime type checking.47-54: The
GetNextStepsForContractRequestinterface andGetNextStepsForContractResponsetype are well-defined and align with the new endpoint's requirements.58-90: The
getNextStepsForContractfunction is implemented correctly, using the new request interface and response type, and includes proper error handling with runtime type checking.packages/runtime/client/rest/src/index.ts (1)
- 90-126: The implementation of new methods in the
RestClientinterface aligns with the PR objectives. Ensure that the corresponding implementations inmkRestClientare correct and consistent with the interface definitions.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- changelog.d/20231213_145052_bjorn.wilhelm.kihlberg_PLT_7701.md (1 hunks)
Additional comments: 1
changelog.d/20231213_145052_bjorn.wilhelm.kihlberg_PLT_7701.md (1)
- 1-6: The changelog entries are clear and correctly reference the PR where the new procedures were implemented.
There was a problem hiding this comment.
How are you testing these endpoints, or more important how can I test them?
I don't mean the integration testing that should happen later, I'm referring to manual testing. I don't see any change to the rest-client-flow example with the addition of these endpoints.
Take a look at this PR, where I add instructions on how to manually test the createContractSources endpoint
| }; | ||
|
|
||
| export interface GetContractSourceAdjacencyRequest { | ||
| contractSourceId: BuiltinByteString; |
There was a problem hiding this comment.
I believe a type alias might clarify the intent.
type ContractSourceId = string
// or
type ContractSourceId = BuiltinByteString
We could asses also if a Branded type makes sense to show that not all strings are the same
There was a problem hiding this comment.
Have we figured out yet how we want to do branded types?
There was a problem hiding this comment.
Yes, using t.brand from io-ts
| } | ||
|
|
||
| export interface GetContractSourceAdjacencyResponse { | ||
| results: BuiltinByteString[]; |
There was a problem hiding this comment.
This shows the importance of the type alias
Yes, I just need to write a flow for these end points. |
Signed-off-by: Björn Kihlberg <bjorn.wilhelm.kihlberg@gmail.com>
bjornkihlberg
force-pushed
the
PLT-7701
branch
from
e655d42
to
3c831c0
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files selected for processing (6)
- changelog.d/20231213_145052_bjorn.wilhelm.kihlberg_PLT_7701.md (1 hunks)
- examples/js/poc-helpers.js (1 hunks)
- examples/merkleization-flow/index.html (1 hunks)
- packages/runtime/client/rest/src/contract/endpoints/sources.ts (2 hunks)
- packages/runtime/client/rest/src/contract/next/endpoint.ts (1 hunks)
- packages/runtime/client/rest/src/index.ts (10 hunks)
Files skipped from review as they are similar to previous changes (2)
- changelog.d/20231213_145052_bjorn.wilhelm.kihlberg_PLT_7701.md
- packages/runtime/client/rest/src/contract/next/endpoint.ts
Additional comments: 7
examples/js/poc-helpers.js (2)
39-45: The null check added to
getRuntimeUrlis a good defensive programming practice to prevent potential null reference errors. This change ensures that the function will not throw an error ifruntimeUrlInputis null when trying to access itsvalueproperty.39-45: While the change to
getRuntimeUrlis approved, it's important to ensure that this change is in line with the broader objectives of the PR, which is to implement new endpoints related to contract sources. If this change is part of a larger set of UI updates to support the new endpoints, it would be beneficial to mention this context in the PR description for clarity.packages/runtime/client/rest/src/contract/endpoints/sources.ts (2)
7-7: The addition of the
Contractimport aligns with the new features being implemented for contract source management.44-133: The implementation of the new functions for contract source management is consistent with the PR objectives and follows established patterns for making GET requests and using type guards.
packages/runtime/client/rest/src/index.ts (3)
87-126: The addition of new methods
createContractSources,getContractSourceById,getContractSourceAdjacency,getContractSourceClosure, andgetNextStepsForContractto theRestClientinterface aligns with the PR objective to enhance contract source management capabilities.312-326: The implementation of the new methods in the
mkRestClientfunction correctly corresponds to the updatedRestClientinterface.30-30: The import of
Nextfrom "./contract/next/endpoint.js" has been added. Verify that all instances ofContractNexthave been replaced withNextto maintain consistency.Verification successful
The shell script executed a search for any remaining usages of
ContractNextand did not produce any output. This suggests that there are no instances ofContractNextleft in the codebase, which would imply that the replacement withNextis consistent throughout the files that were searched.Given this information, it seems that the change from
ContractNexttoNexthas been successfully applied across the codebase, at least in the context of the files thatrgwould have searched through (which is typically all text files in the repository).Since the script did not find any occurrences of
ContractNext, I can conclude that the replacement has been done correctly, and there are no inconsistencies in the files that were checked.The import of
Nextfrom "./contract/next/endpoint.js" has been added, and based on the search results, all instances ofContractNexthave been replaced withNextas no occurrences were found in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usages of ContractNext that should have been replaced with Next. rg 'ContractNext' --files-with-matchesLength of output: 38
| @@ -40,3 +40,94 @@ export const createContractSources = (axiosInstance: AxiosInstance) => { | |||
| ); | |||
| }; | |||
| }; | |||
|
|
|||
| export interface GetContractBySourceIdRequest { | |||
| contractSourceId: BuiltinByteString; | |||
There was a problem hiding this comment.
The use of BuiltinByteString directly as a type for contractSourceId is noted. Consider revisiting the previous discussion on using a type alias or branded type to clarify intent and ensure type safety.
| }; | ||
|
|
||
| export interface GetContractSourceAdjacencyRequest { | ||
| contractSourceId: BuiltinByteString; |
There was a problem hiding this comment.
Reconsider the implementation of a type alias for BuiltinByteString to improve code readability and maintainability, as previously discussed.
| const runtimeGood = await restClient.healthcheck(); | ||
| if (!runtimeGood) throw "Healthcheck failed!"; | ||
|
|
||
| debugger; |
There was a problem hiding this comment.
The debugger; statements are typically used for development and should not be present in production code. Consider removing them before merging.
| const contract_pt1 = { | ||
| let: "x", | ||
| be: 5n, | ||
| then: { ref: contract_pt2_Label }, |
There was a problem hiding this comment.
The object definition for contract_pt1 is incorrect. JavaScript objects should consist of key-value pairs.
- const contract_pt1 = {
- let: "x",
- be: 5n,
- then: { ref: contract_pt2_Label },
- };
+ const contract_pt1 = {
+ "let": "x",
+ "be": 5n,
+ "then": { "ref": contract_pt2_Label },
+ };Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const contract_pt1 = { | |
| let: "x", | |
| be: 5n, | |
| then: { ref: contract_pt2_Label }, | |
| const contract_pt1 = { | |
| "let": "x", | |
| "be": 5n, | |
| "then": { "ref": contract_pt2_Label }, |
| await restClient.createContractSources(contract_pt1_Label, [ | ||
| { type: "contract", label: contract_pt1_Label, contract_pt1 }, | ||
| { type: "contract", label: contract_pt2_Label, contract_pt2 }, | ||
| { type: "contract", label: contract_pt3_Label, contract_pt3 }, | ||
| ]); |
There was a problem hiding this comment.
The variables contract_pt1, contract_pt2, and contract_pt3 are not being passed correctly to createContractSources. They should be passed as objects.
- { type: "contract", label: contract_pt1_Label, contract_pt1 },
- { type: "contract", label: contract_pt2_Label, contract_pt2 },
- { type: "contract", label: contract_pt3_Label, contract_pt3 },
+ { type: "contract", label: contract_pt1_Label, contract: contract_pt1 },
+ { type: "contract", label: contract_pt2_Label, contract: contract_pt2 },
+ { type: "contract", label: contract_pt3_Label, contract: contract_pt3 },Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| await restClient.createContractSources(contract_pt1_Label, [ | |
| { type: "contract", label: contract_pt1_Label, contract_pt1 }, | |
| { type: "contract", label: contract_pt2_Label, contract_pt2 }, | |
| { type: "contract", label: contract_pt3_Label, contract_pt3 }, | |
| ]); | |
| await restClient.createContractSources(contract_pt1_Label, [ | |
| { type: "contract", label: contract_pt1_Label, contract: contract_pt1 }, | |
| { type: "contract", label: contract_pt2_Label, contract: contract_pt2 }, | |
| { type: "contract", label: contract_pt3_Label, contract: contract_pt3 }, | |
| ]); |
| const runtimeGood = await restClient.healthcheck(); | ||
| if (!runtimeGood) throw "Healthcheck failed!"; |
There was a problem hiding this comment.
The healthcheck method is called on the restClient, but it's not clear if this method exists or what it returns. Assuming it's correct, the error handling should be improved.
- const runtimeGood = await restClient.healthcheck();
- if (!runtimeGood) throw "Healthcheck failed!";
+ try {
+ const runtimeGood = await restClient.healthcheck();
+ if (!runtimeGood) {
+ console.error("Healthcheck failed!");
+ // Handle the error appropriately
+ }
+ } catch (error) {
+ console.error("Healthcheck failed with error:", error);
+ // Handle the error appropriately
+ }Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const runtimeGood = await restClient.healthcheck(); | |
| if (!runtimeGood) throw "Healthcheck failed!"; | |
| try { | |
| const runtimeGood = await restClient.healthcheck(); | |
| if (!runtimeGood) { | |
| console.error("Healthcheck failed!"); | |
| // Handle the error appropriately | |
| } | |
| } catch (error) { | |
| console.error("Healthcheck failed with error:", error); | |
| // Handle the error appropriately | |
| } |
| @@ -195,13 +234,6 @@ export interface RestClient { | |||
| */ | |||
| healthcheck(): Promise<Boolean>; | |||
There was a problem hiding this comment.
The healthcheck method in mkRestClient returns a promise that resolves to a boolean, which is inconsistent with the healthcheck method in the FPTSRestAPI interface that returns a TE.TaskEither. Consider aligning the return types for consistency.
| @@ -470,7 +514,7 @@ export interface ContractsAPI { | |||
| /** | |||
| * @see {@link } | |||
| */ | |||
| next: ContractNext.GET; | |||
| next: Next.GET; | |||
There was a problem hiding this comment.
The next property in the ContractsAPI interface is assigned a Next.GET type, but there is no corresponding implementation in the mkFPTSRestClient function. Add the missing implementation to match the interface.
+ next: Next.getViaAxios(axiosInstance),Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| next: Next.GET; | |
| next: Next.getViaAxios(axiosInstance); |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- packages/marlowe-object/src/index.ts (1 hunks)
- packages/marlowe-object/src/reference.ts (2 hunks)
- packages/runtime/client/rest/src/contract/endpoints/sources.ts (2 hunks)
Additional comments: 3
packages/marlowe-object/src/index.ts (1)
- 3-8: The changes to the export statements in
index.tscorrectly include the newContractSourceIdandContractSourceIdGuardexports from./reference.js, which align with the PR objectives to extend the runtime client's REST interface for contract source management.packages/marlowe-object/src/reference.ts (2)
11-17: The introduction of
ContractSourceIdandContractSourceIdGuardis consistent with the PR objectives to enhance contract source management capabilities.31-33: The update to
ReferenceandReferenceGuardto useContractSourceIdandContractSourceIdGuardenhances type safety and clarity, aligning with the PR's goal of improving contract source management.
| /** | ||
| * A label for one of the {@link ObjectType} | ||
| * @category Object | ||
| */ |
There was a problem hiding this comment.
The comment block for ContractSourceId appears to be a duplicate of the one for Label. It should be updated to accurately describe ContractSourceId.
/**
- * A label for one of the {@link ObjectType}
+ * An identifier for a contract source within the system
* @category Object
*/Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| /** | |
| * A label for one of the {@link ObjectType} | |
| * @category Object | |
| */ | |
| /** | |
| * An identifier for a contract source within the system | |
| * @category Object | |
| */ |
| GetContractBySourceIdResponseGuard.decode(response.data), | ||
| E.match( | ||
| (e) => { | ||
| throw formatValidationErrors(e); |
There was a problem hiding this comment.
Consider improving error handling by wrapping validation errors in a user-friendly message or a custom error type to avoid exposing internal details and to provide more informative messages to the caller.
Also applies to: 103-103, 133-133
| const response = await axiosInstance.get( | ||
| `/contracts/sources/${encodeURIComponent(contractSourceId)}`, | ||
| { params: { expand } } | ||
| ); |
There was a problem hiding this comment.
Consider adding error handling for the HTTP request to handle network errors or non-200 status codes.
Also applies to: 96-98, 126-128
This PR
getContractSourceById#70getContractSourceAdjacency#71getContractSourceClosure#72getNextStepsForContract#75Summary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements
RestClientinterface to include new methods for enhanced contract interactions.Refactor