PLT-9368: Strict mode for runtime-lifecycle #180
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe overarching change focuses on enhancing type safety and standardizing request object structures across various operations in a Node.js context. This involves transitioning from passing individual parameters to using object parameters with named properties for function calls. Additionally, there's an introduction of new type guards and modifications to support dynamic type checking, reflecting a move towards more robust and error-resistant code practices. Changes
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 your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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: 5
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
packages/language/core/v1/package.jsonis excluded by:!**/*.jsonpackages/marlowe-object/package.jsonis excluded by:!**/*.json
Files selected for processing (25)
- examples/nodejs/src/experimental-features/applicable-inputs.ts (1 hunks)
- examples/nodejs/src/marlowe-object-flow.ts (2 hunks)
- examples/survey-workshop/custodian/index.js (2 hunks)
- examples/vesting-flow/index.html (1 hunks)
- jsdelivr-npm-importmap.js (2 hunks)
- packages/adapter/src/io-ts.ts (2 hunks)
- packages/runtime/client/rest/src/contract/endpoints/collection.ts (6 hunks)
- packages/runtime/client/rest/src/contract/endpoints/singleton.ts (3 hunks)
- packages/runtime/client/rest/src/contract/endpoints/sources.ts (4 hunks)
- packages/runtime/client/rest/src/contract/index.ts (2 hunks)
- packages/runtime/client/rest/src/contract/next/endpoint.ts (2 hunks)
- packages/runtime/client/rest/src/contract/transaction/endpoints/collection.ts (4 hunks)
- packages/runtime/client/rest/src/contract/transaction/endpoints/singleton.ts (3 hunks)
- packages/runtime/client/rest/src/index.ts (15 hunks)
- packages/runtime/client/rest/src/payout/endpoints/collection.ts (2 hunks)
- packages/runtime/client/rest/src/payout/endpoints/singleton.ts (1 hunks)
- packages/runtime/client/rest/src/withdrawal/endpoints/collection.ts (3 hunks)
- packages/runtime/client/rest/src/withdrawal/endpoints/singleton.ts (2 hunks)
- packages/runtime/core/src/textEnvelope.ts (1 hunks)
- packages/runtime/lifecycle/src/browser/index.ts (3 hunks)
- packages/runtime/lifecycle/src/generic/contracts.ts (3 hunks)
- packages/runtime/lifecycle/src/index.ts (2 hunks)
- packages/runtime/lifecycle/src/nodejs/index.ts (1 hunks)
- packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (2 hunks)
- packages/wallet/src/browser/index.ts (2 hunks)
Additional comments: 60
packages/runtime/lifecycle/src/nodejs/index.ts (1)
- 36-36: The
mkRestClientfunction is correctly called with thestrictparameter, ensuring that the strict mode setting is propagated. This is a good practice for maintaining consistency in how strict type checks are applied across different parts of the application.packages/runtime/client/rest/src/payout/endpoints/singleton.ts (1)
- 29-32: The definition of
GetPayoutByIdRequestGuardcorrectly usesassertGuardEqualto ensure that theGetPayoutByIdRequesttype aligns with its runtime validation. This is a good practice for maintaining type safety and ensuring that the data structures used in the application are correctly validated at runtime.packages/runtime/core/src/textEnvelope.ts (1)
- 45-46: The introduction of
HexTransactionWitnessSetGuardas a type guard forHexTransactionWitnessSetis a straightforward and effective way to ensure that values assigned to variables of typeHexTransactionWitnessSetare indeed strings. This is a good practice for enhancing type safety in the application.packages/runtime/client/rest/src/contract/index.ts (3)
- 46-46: The export of
SubmitContractRequestfrom./endpoints/singleton.jsis correctly implemented, ensuring that this type is available for use in other parts of the application. This is a good practice for modularizing type definitions and promoting code reuse.- 50-52: The export of transaction-related request types from
./transaction/endpoints/singleton.jsis correctly implemented. This modular approach to exporting types enhances code maintainability and readability by clearly separating concerns.- 57-57: The addition of
GetTransactionsForContractRequestto the exports from./transaction/endpoints/collection.jsis a good practice, ensuring that this type is available for use where needed in the application. It promotes a consistent and modular approach to managing type definitions.packages/runtime/lifecycle/src/index.ts (1)
- 75-75: The
mkRestClientfunction is correctly called with thestrictparameter, ensuring that the strict mode setting is propagated. This is a good practice for maintaining consistency in how strict type checks are applied across different parts of the application.examples/survey-workshop/custodian/index.js (2)
- 25-27: The modification to call
restClient.getTransactionsForContractwith an object parameter is a good practice. It improves code readability and maintainability by clearly indicating what each parameter represents.- 38-41: Similarly, the modification to call
restClient.getContractTransactionByIdwith an object parameter enhances code readability and maintainability. Using named properties in function calls makes the code more self-documenting and easier to understand.packages/runtime/lifecycle/src/browser/index.ts (1)
- 93-93: The
mkRestClientfunction is correctly called with thestrictparameter, ensuring that the strict mode setting is propagated. This is a good practice for maintaining consistency in how strict type checks are applied across different parts of the application, even in a browser environment.packages/adapter/src/io-ts.ts (2)
- 100-102: The introduction of the
strictDynamicTypeCheckfunction is a good practice for validating the type of thestrictparameter in a dynamically typed context. This function enhances type safety by ensuring thatstrictis always a boolean.- 104-110: The addition of the
InvalidTypeErrorclass is a good practice for error handling. It extends theErrorclass with a constructor that includes parameters for errors and a message, allowing for more descriptive error messages related to type validation failures.packages/runtime/client/rest/src/contract/next/endpoint.ts (1)
- 54-66: The definition of
GetNextStepsForContractRequestGuardcorrectly usesassertGuardEqualto ensure that theGetNextStepsForContractRequesttype aligns with its runtime validation. This is a good practice for maintaining type safety and ensuring that the data structures used in the application are correctly validated at runtime.packages/runtime/client/rest/src/withdrawal/endpoints/singleton.ts (2)
- 34-41: The definition of
GetWithdrawalByIdRequestGuardcorrectly usesassertGuardEqualto ensure that theGetWithdrawalByIdRequesttype aligns with its runtime validation. This is a good practice for maintaining type safety and ensuring that the data structures used in the application are correctly validated at runtime.- 52-58: Similarly, the definition of
SubmitWithdrawalRequestGuardcorrectly usesassertGuardEqualto ensure that theSubmitWithdrawalRequesttype aligns with its runtime validation. This enhances type safety by ensuring that the data structures used in the application are correctly validated at runtime.packages/runtime/client/rest/src/contract/endpoints/singleton.ts (2)
- 74-84: The definition of
SubmitContractRequestGuardcorrectly usesassertGuardEqualto ensure that theSubmitContractRequesttype aligns with its runtime validation. This is a good practice for maintaining type safety and ensuring that the data structures used in the application are correctly validated at runtime.- 87-87: The replacement of
submitContractViaAxioswithsubmitContractand the update to thePUTtype to include new parameters and guards forHexTransactionWitnessSetandHexTransactionWitnessSetGuardare correctly implemented. This enhances the clarity and maintainability of the code by using more descriptive function names and ensuring type safety through the use of guards.packages/runtime/client/rest/src/contract/transaction/endpoints/singleton.ts (2)
- 39-49: The definition of
GetContractTransactionByIdRequestGuardcorrectly usesassertGuardEqualto ensure that theGetContractTransactionByIdRequesttype aligns with its runtime validation. This is a good practice for maintaining type safety and ensuring that the data structures used in the application are correctly validated at runtime.- 73-85: Similarly, the definition of
SubmitContractTransactionRequestGuardcorrectly usesassertGuardEqualto ensure that theSubmitContractTransactionRequesttype aligns with its runtime validation. This enhances type safety by ensuring that the data structures used in the application are correctly validated at runtime.packages/runtime/client/rest/src/payout/endpoints/collection.ts (1)
- 35-47: The definition of
GetPayoutsRequestGuardcorrectly usesassertGuardEqualto ensure that theGetPayoutsRequesttype aligns with its runtime validation. This is a good practice for maintaining type safety and ensuring that the data structures used in the application are correctly validated at runtime.packages/runtime/client/rest/src/contract/endpoints/sources.ts (4)
- 23-28: The definition of
CreateContractSourcesRequestGuardcorrectly usesassertGuardEqualto ensure that theCreateContractSourcesRequesttype aligns with its runtime validation. This is a good practice for maintaining type safety and ensuring that the data structures used in the application are correctly validated at runtime.- 68-73: The definition of
GetContractBySourceIdRequestGuardcorrectly usesassertGuardEqualto ensure that theGetContractBySourceIdRequesttype aligns with its runtime validation. This enhances type safety by ensuring that the data structures used in the application are correctly validated at runtime.- 106-107: Similarly, the definition of
GetContractSourceAdjacencyRequestGuardcorrectly usesassertGuardEqualto ensure that theGetContractSourceAdjacencyRequesttype aligns with its runtime validation. This is a good practice for maintaining type safety and ensuring that the data structures used in the application are correctly validated at runtime.- 139-140: The definition of
GetContractSourceClosureRequestGuardcorrectly usesassertGuardEqualto ensure that theGetContractSourceClosureRequesttype aligns with its runtime validation. This enhances type safety by ensuring that the data structures used in the application are correctly validated at runtime.jsdelivr-npm-importmap.js (4)
- 27-28: The addition of the mapping for
@marlowe.io/language-core-v1/contractis correctly implemented.- 31-32: The addition of the mapping for
@marlowe.io/language-core-v1/inputsis correctly implemented.- 35-36: The addition of the mapping for
@marlowe.io/language-core-v1/participantsis correctly implemented.- 83-84: The addition of the mapping for
@marlowe.io/marlowe-object/objectis correctly implemented.packages/runtime/client/rest/src/contract/transaction/endpoints/collection.ts (4)
- 18-18: The addition of
AddressBech32Guardis correctly implemented and enhances type safety.- 34-39: The addition of
ItemRangeGuard,PageGuard, andInputGuarddeclarations is correctly implemented, improving the structure and type safety of the code.- 88-103: The modifications to the
GetTransactionsForContractRequestinterface and the addition ofGetTransactionsForContractRequestGuardare correctly implemented, aligning with best practices for type safety and code maintainability.- 145-162: The modifications to the
ApplyInputsToContractRequestinterface and the addition ofApplyInputsToContractRequestGuardare correctly implemented, enhancing type safety and code structure.packages/wallet/src/browser/index.ts (3)
- 3-3: The addition of
io-tsimport is correctly implemented and is essential for runtime type validation.- 6-6: The addition of
assertGuardEqualandproxyimports from@marlowe.io/adapter/io-tsis correctly implemented, supporting the new type guard functionality.- 27-31: The introduction of
SupportedWalletNameGuardusingio-tsfor type validation is correctly implemented, enhancing type safety within the module.packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (2)
- 252-252: The modification to the
getContractByIdfunction call to pass an object with thecontractIdproperty is correctly implemented, aligning with the updated API signature.- 271-271: The modification to the
getContractByIdfunction call withingetMarloweStatefromAnActiveContractto pass an object with thecontractIdproperty is correctly implemented, enhancing code readability and maintainability.packages/runtime/lifecycle/src/generic/contracts.ts (3)
- 59-67: The modification to the
getTransactionsForContractfunction call to pass an object with thecontractIdproperty is correctly implemented, aligning with the updated API signature.- 137-139: The modification to the
createContractSourcesfunction call to pass an object with thebundleproperty is correctly implemented, enhancing code readability and maintainability.- 153-156: The modification to the
submitContractfunction call to pass an object with thecontractIdandtxEnvelopeproperties is correctly implemented, aligning with the updated API signature.examples/vesting-flow/index.html (2)
- 143-143: The modification to the
getContractByIdfunction call to pass an object with thecontractIdproperty is correctly implemented, aligning with the updated API signature.- 150-157: The modifications to the
getTransactionsForContractandgetContractTransactionByIdfunction calls to pass objects with named properties are correctly implemented, enhancing code readability and maintainability.examples/nodejs/src/experimental-features/applicable-inputs.ts (1)
- 125-125: The change to call
restClient.getContractByIdwith an object containingcontractIdas a property, instead of passingcontractIddirectly, aligns with the PR's goal of improving type safety and developer experience by standardizing function calls.examples/nodejs/src/marlowe-object-flow.ts (1)
- 588-596: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [591-615]
The modifications to use object parameters for
getContractByIdandcreateContractSourcescalls are consistent with the PR's objectives to improve type safety and standardize function calls.packages/runtime/client/rest/src/contract/endpoints/collection.ts (3)
- 35-35: The addition of
AddressBech32Guardto the imports supports the PR's objective of improving type safety through stricter type checking.- 52-52: The addition of
RoleNameGuardto the imports further enhances type safety by ensuring role names adhere to expected formats.- 80-88: The definition of
GetContractsRequestGuardcorrectly uses the newly importedAddressBech32GuardandRoleNameGuard, aligning with the PR's focus on type safety.packages/runtime/client/rest/src/index.ts (13)
- 32-39: The addition of
CompatibleRuntimeVersionGuard,RuntimeVersion,InvalidTypeError, andstrictDynamicTypeCheckimports aligns with the PR's objective to enhance type safety and introduce strict mode functionality.- 108-108: The change in the
createContractSourcesmethod signature to accept aSources.CreateContractSourcesRequestobject instead of aContractBundleimproves the method's clarity and type safety by using structured request objects.- 154-155: The modification of the
getContractByIdmethod to accept a request object enhances the method's usability and consistency with other SDK methods.- 161-161: Updating the
submitContractmethod to take a request object aligns with the PR's goal of improving parameter handling and type safety.- 171-171: The change in the
getTransactionsForContractmethod signature to use a request object is a positive step towards standardizing method interfaces across the SDK.- 191-191: Modifying the
submitContractTransactionmethod to accept a request object is consistent with the PR's objectives of enhancing type safety and improving developer experience.- 202-202: The update to the
getContractTransactionByIdmethod signature to use a request object instead of individual parameters is a good practice for improving code readability and maintainability.- 231-231: Changing the
getWithdrawalByIdmethod to accept a request object is in line with the PR's aim to standardize method calls and improve type safety.- 237-237: The modification of the
submitWithdrawalmethod to use a request object instead of individual parameters aligns with the PR's objectives and enhances the method's clarity.- 256-291: The introduction of
mkRestClientArgumentDynamicTypeCheckand the modifications tomkRestClientto include dynamic type checking and strict mode are significant improvements. They enforce type safety and provide developers with more informative error messages, aligning with the PR's goals. However, ensure that the performance impact of these checks, especially in non-strict mode, is minimal and documented.- 317-327: The implementation of strict type checking in the
getContractsmethod, usingContracts.GetContractsRequestGuard, is a good practice that enhances type safety and error handling in the SDK.- 341-351: Implementing strict type checking in the
getContractByIdmethod usingContract.GetContractByIdRequestguard is consistent with the PR's objectives to improve type safety and developer experience.- 355-367: The addition of strict type checking in the
buildCreateContractTxmethod usingContracts.BuildCreateContractTxRequestGuardaligns with the PR's goal of enhancing type safety and improving error handling.
| @@ -122,7 +122,7 @@ export async function getApplicableActions( | |||
| contractId: ContractId, | |||
| environment?: Environment | |||
| ): Promise<ApplicableAction[]> { | |||
| const contractDetails = await restClient.getContractById(contractId); | |||
| const contractDetails = await restClient.getContractById({ contractId }); | |||
There was a problem hiding this comment.
The use of await inside getApplicableActions without a try-catch block for error handling could lead to unhandled promise rejections if restClient.getContractById fails. Consider adding error handling to improve robustness.
+ try {
const contractDetails = await restClient.getContractById({ contractId });
+ } catch (error) {
+ console.error("Failed to get contract details:", error);
+ throw error; // Rethrow or handle as appropriate for your application logic
+ }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.
Suggested change
| const contractDetails = await restClient.getContractById({ contractId }); | |
| try { | |
| const contractDetails = await restClient.getContractById({ contractId }); | |
| } catch (error) { | |
| console.error("Failed to get contract details:", error); | |
| throw error; // Rethrow or handle as appropriate for your application logic | |
| } |
Comment on lines
588
to
596
| contractId: ContractId | ||
| ): Promise<ValidationResults> { | ||
| // First we try to fetch the contract details and the required tags | ||
| const contractDetails = | ||
| await lifecycle.restClient.getContractById(contractId); | ||
| const contractDetails = await lifecycle.restClient.getContractById({ | ||
| contractId, | ||
| }); | ||
|
|
||
| const scheme = extractSchemeFromTags(contractDetails.tags); | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [591-615]
Consider adding error handling for the asynchronous calls to getContractById, createContractSources, and getContractSourceById to manage potential failures gracefully.
+ try {
const contractDetails = await lifecycle.restClient.getContractById({ contractId });
...
const { contractSourceId } = await lifecycle.restClient.createContractSources({ bundle: contractBundle });
...
const initialContract = await lifecycle.restClient.getContractSourceById({ contractSourceId });
+ } catch (error) {
+ console.error("Error fetching contract details or sources:", error);
+ throw error; // Rethrow or handle as appropriate for your application logic
+ }
Comment on lines
+26
to
+32
| strict: unknown = true | ||
| ) { | ||
| if (!strictDynamicTypeCheck(strict)) { | ||
| throw new InvalidTypeError( | ||
| [], | ||
| `Invalid type for argument 'strict', expected boolean but got ${strict}` | ||
| ); |
There was a problem hiding this comment.
The validation for the strict parameter uses a custom type check (strictDynamicTypeCheck) but does not correctly check the type of strict. It should directly check if strict is a boolean instead of using a custom function that is not shown in this context.
- if (!strictDynamicTypeCheck(strict)) {
+ if (typeof strict !== 'boolean') {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.
Suggested change
| strict: unknown = true | |
| ) { | |
| if (!strictDynamicTypeCheck(strict)) { | |
| throw new InvalidTypeError( | |
| [], | |
| `Invalid type for argument 'strict', expected boolean but got ${strict}` | |
| ); | |
| strict: unknown = true | |
| ) { | |
| if (typeof strict !== 'boolean') { | |
| throw new InvalidTypeError( | |
| [], | |
| `Invalid type for argument 'strict', expected boolean but got ${strict}` | |
| ); |
Comment on lines
+67
to
+71
| if (!strictDynamicTypeCheck(strict)) { | ||
| throw new InvalidTypeError( | ||
| [], | ||
| `Invalid type for argument 'strict', expected boolean but got ${strict}` | ||
| ); |
There was a problem hiding this comment.
The validation for the strict parameter within the mkRuntimeLifecycle function does not directly check if strict is a boolean. Instead, it relies on a custom type check (strictDynamicTypeCheck) that is not shown in this context. Directly checking the type of strict would be more straightforward and reliable.
- if (!strictDynamicTypeCheck(strict)) {
+ if (typeof strict !== 'boolean') {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.
Suggested change
| if (!strictDynamicTypeCheck(strict)) { | |
| throw new InvalidTypeError( | |
| [], | |
| `Invalid type for argument 'strict', expected boolean but got ${strict}` | |
| ); | |
| if (typeof strict !== 'boolean') { | |
| throw new InvalidTypeError( | |
| [], | |
| `Invalid type for argument 'strict', expected boolean but got ${strict}` | |
| ); |
Comment on lines
+77
to
+81
| if (!strictDynamicTypeCheck(strict)) { | ||
| throw new InvalidTypeError( | ||
| [], | ||
| `Invalid type for argument 'strict', expected boolean but got ${strict}` | ||
| ); |
There was a problem hiding this comment.
The validation for the strict parameter within the mkRuntimeLifecycle function does not directly check if strict is a boolean. Instead, it relies on a custom type check (strictDynamicTypeCheck) that is not shown in this context. Directly checking the type of strict would be more straightforward and reliable.
- if (!strictDynamicTypeCheck(strict)) {
+ if (typeof strict !== 'boolean') {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.
Suggested change
| if (!strictDynamicTypeCheck(strict)) { | |
| throw new InvalidTypeError( | |
| [], | |
| `Invalid type for argument 'strict', expected boolean but got ${strict}` | |
| ); | |
| if (typeof strict !== 'boolean') { | |
| throw new InvalidTypeError( | |
| [], | |
| `Invalid type for argument 'strict', expected boolean but got ${strict}` | |
| ); |
|
@bjornkihlberg can you squash all these |
hrajchert
reviewed
Jan 29, 2024
Comment on lines
57
to
71
| }, | ||
| "./contract": { | ||
| "import": "./dist/esm/contract.js", | ||
| "require": "./dist/bundled/cjs/contract.cjs", | ||
| "types": "./dist/esm/contract.d.ts" | ||
| }, | ||
| "./participants": { | ||
| "import": "./dist/esm/participants.js", | ||
| "require": "./dist/bundled/cjs/participants.cjs", | ||
| "types": "./dist/esm/participants.d.ts" | ||
| }, | ||
| "./inputs": { | ||
| "import": "./dist/esm/inputs.js", | ||
| "require": "./dist/bundled/cjs/inputs.cjs", | ||
| "types": "./dist/esm/inputs.d.ts" |
There was a problem hiding this comment.
Why are you exporting these?
There was a problem hiding this comment.
I had to in order to be able to use the rest client from the browser. Maybe we can avoid it. I'll look into it.
Comment on lines
51
to
52
| import { ContractGuard } from "@marlowe.io/language-core-v1/contract"; | ||
| import { RoleNameGuard } from "@marlowe.io/language-core-v1/participants"; |
There was a problem hiding this comment.
these should be
import * as G from "@marlowe.io/language-core-v1/guards
G.Contract
G.RoleName
There was a problem hiding this comment.
This may remove the need for some of the exported modules.
| `Invalid argument to getContracts(${request})` | ||
| ); | ||
| } else { | ||
| const test: typeof request = result.right; |
There was a problem hiding this comment.
It's unused at runtime but it helps ensure that you are using the correct guard. I think there are better ways of solving this. I'm going to look into it.
Comment on lines
341
to
351
| if (strict) { | ||
| const result = Contract.GetContractByIdRequest.decode(request); | ||
| if (result._tag === "Left") { | ||
| throw new InvalidTypeError( | ||
| result.left, | ||
| `Invalid argument to getContractById(${request})` | ||
| ); | ||
| } else { | ||
| const test: typeof request = result.right; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this pattern could be captured by a function assertRequest(methodName, guard, request, strict)
There was a problem hiding this comment.
Yes, I was thinking the same.
bjornkihlberg
force-pushed
the
PLT-9368
branch
from
6c57cbe
to
0a5e457
Compare
bjornkihlberg
force-pushed
the
PLT-9368
branch
from
0a5e457
to
c443dbc
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
packages/marlowe-object/package.jsonis excluded by:!**/*.json
Files selected for processing (26)
- changelog.d/20240126_162752_bjorn.wilhelm.kihlberg_PLT_9368.md (1 hunks)
- examples/nodejs/src/experimental-features/applicable-inputs.ts (1 hunks)
- examples/nodejs/src/marlowe-object-flow.ts (2 hunks)
- examples/survey-workshop/custodian/index.js (2 hunks)
- examples/vesting-flow/index.html (1 hunks)
- jsdelivr-npm-importmap.js (1 hunks)
- packages/adapter/src/io-ts.ts (2 hunks)
- packages/runtime/client/rest/src/contract/endpoints/collection.ts (7 hunks)
- packages/runtime/client/rest/src/contract/endpoints/singleton.ts (3 hunks)
- packages/runtime/client/rest/src/contract/endpoints/sources.ts (4 hunks)
- packages/runtime/client/rest/src/contract/index.ts (2 hunks)
- packages/runtime/client/rest/src/contract/next/endpoint.ts (2 hunks)
- packages/runtime/client/rest/src/contract/transaction/endpoints/collection.ts (4 hunks)
- packages/runtime/client/rest/src/contract/transaction/endpoints/singleton.ts (3 hunks)
- packages/runtime/client/rest/src/index.ts (15 hunks)
- packages/runtime/client/rest/src/payout/endpoints/collection.ts (2 hunks)
- packages/runtime/client/rest/src/payout/endpoints/singleton.ts (1 hunks)
- packages/runtime/client/rest/src/withdrawal/endpoints/collection.ts (3 hunks)
- packages/runtime/client/rest/src/withdrawal/endpoints/singleton.ts (2 hunks)
- packages/runtime/core/src/textEnvelope.ts (1 hunks)
- packages/runtime/lifecycle/src/browser/index.ts (3 hunks)
- packages/runtime/lifecycle/src/generic/contracts.ts (3 hunks)
- packages/runtime/lifecycle/src/index.ts (2 hunks)
- packages/runtime/lifecycle/src/nodejs/index.ts (1 hunks)
- packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (2 hunks)
- packages/wallet/src/browser/index.ts (2 hunks)
Files skipped from review as they are similar to previous changes (24)
- changelog.d/20240126_162752_bjorn.wilhelm.kihlberg_PLT_9368.md
- examples/nodejs/src/experimental-features/applicable-inputs.ts
- examples/nodejs/src/marlowe-object-flow.ts
- examples/survey-workshop/custodian/index.js
- examples/vesting-flow/index.html
- jsdelivr-npm-importmap.js
- packages/adapter/src/io-ts.ts
- packages/runtime/client/rest/src/contract/endpoints/collection.ts
- packages/runtime/client/rest/src/contract/endpoints/singleton.ts
- packages/runtime/client/rest/src/contract/endpoints/sources.ts
- packages/runtime/client/rest/src/contract/index.ts
- packages/runtime/client/rest/src/contract/transaction/endpoints/collection.ts
- packages/runtime/client/rest/src/contract/transaction/endpoints/singleton.ts
- packages/runtime/client/rest/src/payout/endpoints/collection.ts
- packages/runtime/client/rest/src/payout/endpoints/singleton.ts
- packages/runtime/client/rest/src/withdrawal/endpoints/collection.ts
- packages/runtime/client/rest/src/withdrawal/endpoints/singleton.ts
- packages/runtime/core/src/textEnvelope.ts
- packages/runtime/lifecycle/src/browser/index.ts
- packages/runtime/lifecycle/src/generic/contracts.ts
- packages/runtime/lifecycle/src/index.ts
- packages/runtime/lifecycle/src/nodejs/index.ts
- packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts
- packages/wallet/src/browser/index.ts
Additional comments: 7
packages/runtime/client/rest/src/contract/next/endpoint.ts (2)
- 54-66: The
GetNextStepsForContractRequestGuardcorrectly usesio-tsfor runtime type validation, ensuring that the request object adheres to the expected structure. This is a good practice for enhancing type safety in dynamic environments.- 67-67: The
getNextStepsForContractfunction correctly decodes and validates the response data using theGetNextStepsForContractResponseGuard, throwing formatted validation errors if decoding fails. This approach is consistent with best practices for error handling and data validation.packages/runtime/client/rest/src/index.ts (5)
- 32-39: The addition of
CompatibleRuntimeVersionGuard,RuntimeVersion,InvalidTypeError, andstrictDynamicTypeCheckimports supports the new strict mode functionality, aligning with the PR's objective to enhance type safety and developer experience.- 108-108: The modification of the
createContractSourcesmethod signature to accept a request object instead of individual parameters is a positive change. It improves the method's usability and consistency with the rest of the API, aligning with the PR's objectives.- 256-261: The
mkRestClientArgumentDynamicTypeCheckfunction provides a runtime check for thebaseURLargument, ensuring it's a string when strict mode is enabled. This is a good practice for type safety.- 280-291: The dynamic type checking for
strictandbaseURLarguments inmkRestClientenhances error reporting and type safety, aligning with the PR's objectives to improve developer experience through stricter runtime checks.- 317-327: The implementation of strict mode checks in methods like
getContractsis consistent and correctly throwsInvalidTypeErrorwhen the request object does not match the expected type. This approach significantly improves error handling and developer feedback.
| @@ -1,15 +1,18 @@ | |||
| import { AxiosInstance } from "axios"; | |||
| import * as E from "fp-ts/lib/Either.js"; | |||
| import * as TE from "fp-ts/lib/TaskEither.js"; | |||
| import * as G from "@marlowe.io/language-core-v1/guards"; | |||
There was a problem hiding this comment.
The import * as G from "@marlowe.io/language-core-v1/guards"; is not used in the file.
- import * as G from "@marlowe.io/language-core-v1/guards";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.
Suggested change
| import * as G from "@marlowe.io/language-core-v1/guards"; |
| import { Environment, Party } from "@marlowe.io/language-core-v1"; | ||
| import { Next } from "@marlowe.io/language-core-v1/next"; | ||
| import { stringify } from "qs"; | ||
| import { DecodingError } from "@marlowe.io/adapter/codec"; | ||
| import { posixTimeToIso8601 } from "@marlowe.io/adapter/time"; | ||
| import { assertGuardEqual, proxy } from "@marlowe.io/adapter/io-ts"; | ||
| import * as t from "io-ts/lib/index.js"; |
There was a problem hiding this comment.
The assertGuardEqual import is not used in this file. Consider removing it to clean up unused imports.
- import { assertGuardEqual, proxy } from "@marlowe.io/adapter/io-ts";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.
Suggested change
| import * as t from "io-ts/lib/index.js"; | |
| import * as t from "io-ts/lib/index.js"; |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- packages/adapter/src/io-ts.ts (2 hunks)
- packages/runtime/client/rest/src/contract/endpoints/collection.ts (7 hunks)
- packages/runtime/client/rest/src/index.ts (10 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/adapter/src/io-ts.ts
- packages/runtime/client/rest/src/contract/endpoints/collection.ts
Additional comments: 11
packages/runtime/client/rest/src/index.ts (11)
- 14-14: The removal of
ContractBundleimport is not directly visible in the provided code, but its usage seems to have been replaced with request objects in method signatures. Ensure that all instances whereContractBundlewas previously used have been appropriately updated.Verification successful
The verification process focused on ensuring that the removal or modification of
ContractBundleinrest/src/index.tswas handled appropriately. The presence ofContractBundlein other parts of the project, as revealed by the script output, does not directly indicate an issue with the specific changes inrest/src/index.ts. The original review comment remains relevant, emphasizing the need to ensure that all instances whereContractBundlewas previously used in the context ofrest/src/index.tshave been appropriately updated. Based on the information provided, there is no direct evidence to suggest that the changes inrest/src/index.tsregardingContractBundlewere incorrectly done. Therefore, the verification process concludes with the understanding that the specific changes made torest/src/index.tsare consistent with the project's current state and usage patterns ofContractBundle.* 33-40: New imports for `CompatibleRuntimeVersionGuard`, `RuntimeVersion`, `InvalidTypeError`, and `strictDynamicTypeCheck` have been added. Verify that these are correctly utilized in the context of enabling strict mode and dynamic type checking throughout the file. * 110-110: The `createContractSources` method signature has been modified to accept a `Sources.CreateContractSourcesRequest` object instead of a `ContractBundle`. This change aligns with the PR's objective to standardize function calls and improve code readability by using request objects. * 155-157: The `getContractById` method signature has been updated to accept a `Contract.GetContractByIdRequest` object. This change enhances the method's clarity and consistency with other updated method signatures. * 163-163: The `submitContract` method now takes a `Contract.SubmitContractRequest` object, aligning with the PR's goal of using request objects for better parameter handling and type safety. * 173-173: The `getTransactionsForContract` method signature has been updated to use a `Transactions.GetTransactionsForContractRequest` object, which is consistent with the PR's aim to improve function call standardization. * 193-193: The `submitContractTransaction` method signature change to accept a `Transaction.SubmitContractTransactionRequest` object is in line with the PR's objectives to enhance type safety and parameter handling. * 233-233: The `getWithdrawalById` method signature has been modified to accept a `Withdrawal.GetWithdrawalByIdRequest` object, further standardizing method signatures across the SDK. * 239-239: The `submitWithdrawal` method signature has been updated to use a `Withdrawal.SubmitWithdrawalRequest` object, aligning with the PR's goals for improved parameter handling and type safety. * 258-277: The addition of `mkRestClientArgumentDynamicTypeCheck` and `withDynamicTypeCheck` functions introduces dynamic type checking based on the `strict` mode setting. This is a crucial part of the PR's objective to enhance type safety and developer experience by providing more informative error messages during development. Ensure that these functions are correctly implemented and used throughout the file. * 255-313: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [291-324]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to ContractBundle in the project. rg "ContractBundle" --type tsLength of output: 1292
The
mkRestClientfunction has been overloaded to support an optionalstrictboolean parameter, enabling dynamic type checking. This change is central to the PR's goal of introducing a strict mode for runtime lifecycle management. Verify that the strict mode is correctly applied and that the dynamic type checks are performed as expected.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Examples
Examples of improvements to developer experience
Summary by CodeRabbit
New Features
@marlowe.ionamespace, facilitating easier module access and management.Refactor
Documentation
Bug Fixes