Fix how the ContractId and TxId branded types are printed in the documentation #185
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 changes focus on enhancing type safety and consistency by introducing type guards like 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: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
typedoc.jsonis excluded by:!**/*.json
Files selected for processing (8)
- packages/adapter/src/io-ts.ts (1 hunks)
- packages/runtime/client/rest/src/contract/transaction/details.ts (3 hunks)
- packages/runtime/client/rest/src/contract/transaction/endpoints/collection.ts (2 hunks)
- packages/runtime/client/rest/src/contract/transaction/endpoints/singleton.ts (3 hunks)
- packages/runtime/client/rest/src/contract/transaction/header.ts (2 hunks)
- packages/runtime/core/src/contract/id.ts (2 hunks)
- packages/runtime/core/src/payout/index.ts (3 hunks)
- packages/runtime/core/src/tx/id.ts (1 hunks)
Additional comments: 17
packages/runtime/core/src/tx/id.ts (3)
- 4-6: The
TxIdBrandinterface is correctly defined with a unique symbol to brand theTxIdtype. This approach ensures type safety by distinguishingTxIdfrom regular strings in the type system.- 14-14: The
TxIdtype is properly branded using theTxIdBrand, aligning with the objective to enhance type safety for branded types in the SDK.- 22-22: The
txIdfunction correctly utilizesunsafeEitherto decode a string into aTxId, leveraging theTxIdGuard. This approach simplifies the creation ofTxIdinstances from strings while handling potential decoding errors.packages/runtime/core/src/contract/id.ts (3)
- 5-5: The introduction of the
ContractIdBrandinterface with a unique symbol for branding theContractIdtype is a good practice for enhancing type safety and ensuringContractIdis distinct from other string types.- 25-25: The
ContractIdtype is correctly defined as a branded string using theContractIdBrand, aligning with the objective to improve type safety for branded types in the SDK.- 32-32: The
contractIdToTxIdfunction is updated to include thetxIdfunction in its flow, effectively converting aContractIdto aTxId. This change enhances the function's utility by incorporating the newly introduced branded types. Ensure that thesplit("#")operation is appropriate for the expected format ofContractId.packages/runtime/client/rest/src/contract/transaction/header.ts (1)
- 50-50: Updating the
transactionIdfield in theTxHeaderinterface to useTxIdGuardinstead ofTxIddirectly is a positive change for enhancing type safety. This ensures that thetransactionIdfield is validated against theTxIdGuardat runtime.packages/runtime/core/src/payout/index.ts (3)
- 7-7: The addition of
txIdto the import statement from../tx/id.jsis necessary for the subsequent changes in this file, ensuring that thetxIdfunction is available for use in converting payout and withdrawal IDs toTxId.- 18-18: The
payoutIdToTxIdfunction is correctly updated to include thetxIdfunction in its flow, effectively converting aPayoutIdto aTxId. This change aligns with the objective to incorporate branded types effectively.- 30-30: The
withdrawalIdToTxIdfunction is similarly updated to include thetxIdfunction, converting aWithdrawalIdto aTxId. This consistent use of branded types across different modules enhances type safety and clarity.packages/runtime/client/rest/src/contract/transaction/endpoints/singleton.ts (2)
- 49-49: Updating the type guard for
txIdin theGetContractTransactionByIdRequestGuardto useTxIdGuardis a good practice for ensuring that thetxIdfield is validated correctly at runtime, enhancing type safety.- 84-84: Similarly, updating the
transactionIdfield in theSubmitContractTransactionRequestGuardto useTxIdGuardensures consistent and safe handling of transaction IDs across different parts of the SDK.packages/runtime/client/rest/src/contract/transaction/details.ts (2)
- 101-101: Updating the
transactionIdfield in theTransactionDetailsGuardto useTxIdGuardinstead ofTxIddirectly is a positive change for enhancing type safety. This ensures that thetransactionIdfield is validated against theTxIdGuardat runtime.- 112-112: Similarly, updating the
consumingTxfield to useoptionFromNullable(TxIdGuard)instead of directly usingTxIdensures that optional consuming transaction IDs are validated correctly, enhancing type safety and consistency.packages/adapter/src/io-ts.ts (1)
- 153-171: The update to the
preservedBrandfunction to use generic type parameters (C,N,B) and to operate on a codecCinstead oft.Type<A, A, I>is a significant improvement. This change allows for a broader application of the function across different types, enhancing its utility and flexibility in handling branded types. The implementation correctly maintains the branded type's output type, addressing the issue mentioned in the related GitHub issue.packages/runtime/client/rest/src/contract/transaction/endpoints/collection.ts (2)
- 30-30: The addition of the
TxIdGuardimport is necessary for the subsequent change in this file, ensuring that theTxIdGuardis available for use in validating transaction IDs.- 129-129: Updating the
transactionIdfield in theTransactionTextEnvelopetype to useTxIdGuardinstead ofTxIddirectly is a good practice for ensuring that thetransactionIdfield is validated correctly at runtime, enhancing type safety.
Comment on lines
+16
to
+20
| export const TxIdGuard = t.brand( | ||
| t.string, | ||
| (s): s is t.Branded<string, TxIdBrand> => true, | ||
| "TxId" | ||
| ); |
There was a problem hiding this comment.
The TxIdGuard is defined using t.brand to validate strings as TxId types. The predicate function always returns true, which means it does not perform any runtime validation. Consider implementing a more meaningful validation logic to ensure the input string conforms to the expected format of a transaction ID.
- (s): s is t.Branded<string, TxIdBrand> => true,
+ (s): s is t.Branded<string, TxIdBrand> => /^[0-9a-fA-F]{64}$/.test(s),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
| export const TxIdGuard = t.brand( | |
| t.string, | |
| (s): s is t.Branded<string, TxIdBrand> => true, | |
| "TxId" | |
| ); | |
| export const TxIdGuard = t.brand( | |
| t.string, | |
| (s): s is t.Branded<string, TxIdBrand> => /^[0-9a-fA-F]{64}$/.test(s), | |
| "TxId" | |
| ); |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- changelog.d/20240222_130945_hrajchert_fix_branded_output.md (1 hunks)
Additional comments: 1
changelog.d/20240222_130945_hrajchert_fix_branded_output.md (1)
- 1-5: The changelog entry clearly and accurately summarizes the changes made in PR-185, including the fix related to the branding of
ContractIdandTxId. The inclusion of the PR link provides a helpful reference for further details.
nhenin
approved these changes
Feb 23, 2024
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.
In the PR#184 I added the preserveBrand function that helped with the uses of branded types inside recursive data types, but the typing was a little bit off, and it changed the generated documentation from this
to this
This PR:
and moreover it improves the generated output to this:
Note that in order to preserve the type alias (at least in typescript version 4.9.5) we need to define ContractId like this
instead of
Summary by CodeRabbit
TxIdGuard,ContractIdBrand,TxIdBrand) to ensure more robust type checking and validation.