Open Roles & Clean buildCreateContractTx Request #54
Conversation
bjornkihlberg
force-pushed
the
PLT-8261
branch
2 times, most recently
from
440c46b to
ed83d59
Compare
nhenin
modified the milestones:
1-1 feature parity with `marlowe-runtime-web` (runtime) v0.0.5.1,
Open Roles
nhenin
changed the title
nhenin
changed the title
nhenin
changed the title
|
Warning Rate Limit Exceeded@nhenin has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 3 minutes and 58 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe updates reflect a significant refactoring in contract creation and management, with a shift from direct contract creation to building transactions for contract operations. This includes renaming functions and parameters to align with the new transaction-based approach, enhancing role configurations, and updating type guards. Additionally, there's a move towards deprecating older REST client functions in favor of new implementations, and various changes to improve consistency and future-proofing of the codebase. Changes
TipsChat with CodeRabbit Bot (
|
packages/runtime/client/rest/src/contract/endpoints/collection.ts
Outdated
Show resolved
Hide resolved
packages/runtime/client/rest/src/contract/endpoints/collection.ts
Outdated
Show resolved
Hide resolved
![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: 14
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- flake.lock
- packages/runtime/client/rest/package.json
Files selected for processing (32)
- examples/get-my-contract-ids-flow/index.html (1 hunks)
- examples/rest-client-flow/index.html (1 hunks)
- examples/run-lite/index.html (1 hunks)
- examples/survey-workshop/participant/index.js (1 hunks)
- examples/vesting-flow/index.html (1 hunks)
- jsdelivr-npm-importmap.js (1 hunks)
- packages/adapter/src/codec.ts (1 hunks)
- packages/language/core/v1/src/address.ts (1 hunks)
- packages/language/core/v1/src/guards.ts (2 hunks)
- packages/language/core/v1/src/index.ts (1 hunks)
- packages/language/core/v1/src/participants.ts (1 hunks)
- packages/language/core/v1/src/policyId.ts (1 hunks)
- packages/language/core/v1/src/token.ts (2 hunks)
- packages/runtime/client/rest/src/contract/details.ts (2 hunks)
- packages/runtime/client/rest/src/contract/endpoints/collection.ts (8 hunks)
- packages/runtime/client/rest/src/contract/guards.ts (1 hunks)
- packages/runtime/client/rest/src/contract/header.ts (2 hunks)
- packages/runtime/client/rest/src/contract/index.ts (1 hunks)
- packages/runtime/client/rest/src/contract/rolesConfigurations.ts (1 hunks)
- packages/runtime/client/rest/src/index.ts (9 hunks)
- packages/runtime/core/src/address.ts (2 hunks)
- packages/runtime/core/src/index.ts (1 hunks)
- packages/runtime/lifecycle/src/api.ts (2 hunks)
- packages/runtime/lifecycle/src/browser/index.ts (2 hunks)
- packages/runtime/lifecycle/src/generic/contracts.ts (7 hunks)
- packages/runtime/lifecycle/src/generic/payouts.ts (3 hunks)
- packages/runtime/lifecycle/src/generic/runtime.ts (1 hunks)
- packages/runtime/lifecycle/src/nodejs/index.ts (2 hunks)
- packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (2 hunks)
- packages/runtime/lifecycle/test/generic/contracts.e2e.spec.ts (2 hunks)
- packages/runtime/lifecycle/test/generic/payouts.e2e.spec.ts (2 hunks)
- packages/wallet/src/browser/index.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- jsdelivr-npm-importmap.js
Additional comments: 49
examples/get-my-contract-ids-flow/index.html (1)
- 41-45: The change from
contracttocontractOrSourceIdin theruntimeLifeCycle.contracts.createContractcall is consistent with the PR objectives and AI-generated summaries, indicating a broader scope of accepted input for thecreateContractfunction.examples/rest-client-flow/index.html (1)
- 78-88: The changes to the input element's ID and value, as well as the hyperlink URL, are consistent with the PR's objective of renaming the
createContractfunction tobuildCreateContractTx. This suggests an update to the UI to reflect backend changes.examples/run-lite/index.html (1)
- 108-116: The change in the
createContractfunction call to use an object with acontractOrSourceIdfield aligns with the PR's objective to refactor contract creation functions and enhance type safety.examples/survey-workshop/participant/index.js (1)
- 126-128: The change from
contracttocontractOrSourceIdin thecreateContractfunction call is consistent with the PR objectives and the summary provided. This change suggests that the function now accepts a broader range of inputs, which could be either a contract or a source ID.examples/vesting-flow/index.html (1)
- 117-121: The change from
contracttocontractOrSourceIdin thecreateContractfunction call aligns with the PR's objective to modify contract creation functions and introduce new types and guards related to roles and transactions. This change should be reflected in all relevant parts of the codebase wherecreateContractis used.packages/language/core/v1/src/guards.ts (2)
16-16: The addition of the export
PolicyIdGuardasPolicyIdaligns with the PR objectives and the summary provided.63-63: The addition of the export
RoleNameGuardasRoleNameis consistent with the PR objectives and the summary.packages/language/core/v1/src/index.ts (1)
- 58-58: The addition of
RoleNameto the export list inparticipants.jsis correctly reflected in the code.packages/language/core/v1/src/participants.ts (1)
- 32-33: The changes to export
RoleNameandRoleNameGuardare correctly implemented and align with the PR objectives and the provided summary.packages/runtime/client/rest/src/contract/details.ts (1)
- 40-54: The updated documentation comments for the
Get Contract By Idendpoint, including the new category tag, are correctly reflected in the code. This enhances clarity and categorization for the endpoint.packages/runtime/client/rest/src/contract/endpoints/collection.ts (9)
43-44: The renaming of
RolesConfigtoRolesConfigurationand the update of its import path is correctly reflected in the code.255-255: The use of
BuildCreateContractTxRequestin the code suggests that the renaming fromCreateContractRequesthas taken place as mentioned in the summary.508-508: The use of
BuildCreateContractTxResponsein the code suggests that the renaming fromCreateContractResponsehas taken place as mentioned in the summary.205-205: The addition of
ContractOrSourceIdis correctly reflected in the code.211-211: The addition of
ContractOrSourceIdGuardis correctly reflected in the code.66-66: The code for
GetContractsRequestappears to be well-defined and documented.168-168: The code for
GetContractsResponseappears to be well-defined and documented.492-492: The code for
PostContractsRequestappears to be well-defined and documented.549-549: The code for
postViaAxiosappears to be well-defined and documented.packages/runtime/client/rest/src/contract/guards.ts (1)
- 16-27: The export statements for the guards are consistent with the PR objectives and summaries, which indicate the introduction of new types and guards for enhanced type safety and validation.
packages/runtime/client/rest/src/contract/header.ts (2)
22-27: The update to the
@categorytag in the documentation comments forContractHeaderaligns with the PR's objective to enhance documentation and categorization.58-64: The update to the
@categorytag in the documentation comments forContractHeaderGuardis consistent with the changes made toContractHeaderand the PR's objective to improve documentation.packages/runtime/client/rest/src/contract/index.ts (4)
14-51: The summary does not mention the removal of
RolesConfig, but it has been removed as per the hunk. This should be noted in the summary to accurately reflect the changes made to the codebase.39-39: The summary does not mention the addition of
ContractOrSourceId, but it has been added as per the hunk. This should be noted in the summary to accurately reflect the changes made to the codebase.19-19: The summary does not mention the addition of
AddressBech32Brand, but it has been added as per the hunk. This should be noted in the summary to accurately reflect the changes made to the codebase.22-33: The summary does not mention the addition of several new exports such as
ClosedRole,OpenRole,Openness,UsePolicyWithClosedRoleTokens,UsePolicyWithOpenRoleTokens,MintRolesTokens,TokenMetadataFile,TokenMetadata,Recipient,TokenQuantity, andRoleTokenConfiguration, but they have been added as per the hunk. This should be noted in the summary to accurately reflect the changes made to the codebase.packages/runtime/client/rest/src/contract/rolesConfigurations.ts (2)
10-24: The introduction of
AddressBech32BrandandAddressBech32Guardseems to contradict the discussion in the PR comments about replacing theAddressBech32type with one from the Runtime Rest Client Package. Please ensure that this change aligns with the intended refactoring and issue resolution strategy.177-210: The
@categorytags formintRoleanduseMintedRolesfunctions correctly categorize them under both "Roles Configuration" and "Endpoint : Build Create Contract Tx", reflecting their dual relevance.packages/runtime/client/rest/src/index.ts (6)
450-453: The summary indicates that the
RestDItype was renamed, but the hunk shows thatRestDIis a new type. Please clarify if this is a new addition or a renaming of an existing type.234-254: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [237-264]
The changes to
buildCreateContractTxmethod reflect the PR's objective to shift towards building transactions for contract creation. The method now accepts a more complex request object and handles optional properties with spread syntax.
119-127: The summary suggests that the
applyInputsToContractmethod should be renamed tobuildApplyInputsToContractTx, but the method name has not been changed in the hunk. Please confirm if the method should be renamed or if the summary is incorrect.153-161: The summary suggests that the
withdrawPayoutsmethod should be renamed tobuildWithdrawPayoutsTx, but the method name has not been changed in the hunk. Please confirm if the method should be renamed or if the summary is incorrect.57-61: The updates to the
RestClientinterface, including new method signatures and documentation, align with the PR's objectives to enhance the codebase and improve transaction building.79-81: The new method
buildCreateContractTxin theRestClientinterface with updated parameters and return types aligns with the PR's objectives to improve transaction building.packages/runtime/core/src/address.ts (2)
21-27: The addition of the
StakeAddressBech32type and associated functions for wrapping and unwrapping the newtype is correctly implemented and follows the existing pattern forAddressBech32.17-18: The
AddressesAndCollateralstype is correctly defined using theAddressBech32andTxOutReftypes.packages/runtime/core/src/index.ts (1)
- 9-9: The addition of
export * from "./source/id.js";is confirmed and aligns with the PR objectives and the summary provided.packages/runtime/lifecycle/src/api.ts (3)
6-17: The summary does not mention the removal of
RolesConfigimport and the addition ofContractOrSourceIdandRolesConfigurationimports.168-192: The summary does not mention the addition of
tagsandmetadataproperties toCreateContractRequest.51-148: The summary does not mention the addition of
stakeAddress,threadRoleName, androlesproperties toCreateContractRequest.packages/runtime/lifecycle/src/browser/index.ts (1)
- 34-39: The changes to
mkRuntimeLifecyclefunction correctly reflect the PR's objective to introduce a newrestClientparameter and maintain the existingdeprecatedRestAPI. This update aligns with the PR's goal of enhancing the contract creation functions and introducing new types and guards related to roles and transactions.packages/runtime/lifecycle/src/generic/contracts.ts (1)
- 7-10: The summary indicates that the
minUTxODepositDefaultimport was removed, but this change is not visible in the provided hunk. Please verify if this change occurred in another part of the file or if the summary is incorrect.packages/runtime/lifecycle/src/generic/runtime.ts (2)
9-19: The changes to the
mkRuntimeLifecyclefunction, including the addition of therestClientparameter and the renaming ofrestAPItodeprecatedRestAPI, are correctly implemented as per the PR objectives and the summary provided.16-17: The
restClientparameter is being correctly passed to themkContractLifecycleandmkPayoutLifecyclefunctions, aligning with the intended changes.packages/runtime/lifecycle/src/nodejs/index.ts (2)
1-6: The changes to the imports align with the PR's objective to refactor and introduce new types and functions, as well as the summary provided.
19-24: The update to
mkRuntimeLifecycleto include bothdeprecatedRestAPIandrestClientis consistent with the PR's objective to refactor functions and enhance type safety.packages/runtime/lifecycle/test/generic/contracts.e2e.spec.ts (2)
33-39: The update from
contracttocontractOrSourceIdin thecreateContractmethod call aligns with the PR's objective to modify contract creation functions. This change seems to be correctly implemented in the test suite.51-57: The update from
contracttocontractOrSourceIdin thecreateContractmethod call is consistently applied here as well, which is in line with the PR's objective to modify contract creation functions.packages/wallet/src/browser/index.ts (1)
- 77-84: The logic change in the
mkBrowserWalletfunction to compare wallet names in lowercase is a good practice for case-insensitive comparison, ensuring that the function works correctly regardless of the case of the inputwalletName.
packages/runtime/client/rest/src/contract/rolesConfigurations.ts
Outdated
Show resolved
Hide resolved
packages/runtime/client/rest/src/contract/rolesConfigurations.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- flake.lock
- packages/runtime/client/rest/package.json
Files selected for processing (29)
- examples/rest-client-flow/index.html (1 hunks)
- examples/run-lite/index.html (1 hunks)
- examples/survey-workshop/participant/index.js (1 hunks)
- jsdelivr-npm-importmap.js (1 hunks)
- packages/adapter/src/codec.ts (1 hunks)
- packages/language/core/v1/src/address.ts (1 hunks)
- packages/language/core/v1/src/guards.ts (2 hunks)
- packages/language/core/v1/src/index.ts (1 hunks)
- packages/language/core/v1/src/participants.ts (1 hunks)
- packages/language/core/v1/src/policyId.ts (1 hunks)
- packages/language/core/v1/src/token.ts (2 hunks)
- packages/runtime/client/rest/src/contract/details.ts (2 hunks)
- packages/runtime/client/rest/src/contract/endpoints/collection.ts (8 hunks)
- packages/runtime/client/rest/src/contract/guards.ts (1 hunks)
- packages/runtime/client/rest/src/contract/header.ts (2 hunks)
- packages/runtime/client/rest/src/contract/index.ts (1 hunks)
- packages/runtime/client/rest/src/contract/rolesConfigurations.ts (1 hunks)
- packages/runtime/client/rest/src/index.ts (9 hunks)
- packages/runtime/core/src/address.ts (2 hunks)
- packages/runtime/core/src/index.ts (1 hunks)
- packages/runtime/lifecycle/src/api.ts (2 hunks)
- packages/runtime/lifecycle/src/browser/index.ts (2 hunks)
- packages/runtime/lifecycle/src/generic/contracts.ts (7 hunks)
- packages/runtime/lifecycle/src/generic/payouts.ts (3 hunks)
- packages/runtime/lifecycle/src/generic/runtime.ts (1 hunks)
- packages/runtime/lifecycle/src/nodejs/index.ts (2 hunks)
- packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (2 hunks)
- packages/runtime/lifecycle/test/generic/payouts.e2e.spec.ts (2 hunks)
- packages/wallet/src/browser/index.ts (1 hunks)
Files skipped from review due to trivial changes (5)
- jsdelivr-npm-importmap.js
- packages/adapter/src/codec.ts
- packages/language/core/v1/src/guards.ts
- packages/runtime/client/rest/src/contract/details.ts
- packages/runtime/lifecycle/src/browser/index.ts
Files skipped from review as they are similar to previous changes (16)
- examples/rest-client-flow/index.html
- examples/run-lite/index.html
- examples/survey-workshop/participant/index.js
- packages/language/core/v1/src/address.ts
- packages/language/core/v1/src/index.ts
- packages/language/core/v1/src/participants.ts
- packages/language/core/v1/src/policyId.ts
- packages/language/core/v1/src/token.ts
- packages/runtime/client/rest/src/contract/header.ts
- packages/runtime/client/rest/src/index.ts
- packages/runtime/core/src/address.ts
- packages/runtime/core/src/index.ts
- packages/runtime/lifecycle/src/generic/runtime.ts
- packages/runtime/lifecycle/src/nodejs/index.ts
- packages/runtime/lifecycle/test/generic/payouts.e2e.spec.ts
- packages/wallet/src/browser/index.ts
Additional comments: 30
packages/runtime/client/rest/src/contract/endpoints/collection.ts (9)
41-45: The renaming of
RolesConfigtoRolesConfigurationand the update of its import path from../role.jsto../rolesConfigurations.jsis correctly reflected in the code.220-225: The renaming of
CreateContractRequesttoBuildCreateContractTxRequestis implied by the presence ofBuildCreateContractTxRequestWithContractwhich extendsBuildCreateContractTxRequestOptions. However, the direct definition ofBuildCreateContractTxRequestis not visible in the provided code.205-214: The addition of
ContractOrSourceIdandContractOrSourceIdGuardis correctly implemented in the code.555-561: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [534-561]
The update of
CreateContractResponsetoBuildCreateContractTxResponseand the modification of its properties and comments are correctly reflected in the code.
574-589: The update of the type of
postViaAxiostoBuildCreateContractTxEndpointis correctly implemented in the code.66-67: The
GetContractsRequestinterface has been updated with documentation comments and potential refactoring, which is not mentioned in the summary but seems appropriate.168-168: The
GetContractsResponseinterface has been updated with documentation comments and potential refactoring, which is not mentioned in the summary but seems appropriate.517-528: The
PostContractsRequesttype has been updated with documentation comments and potential refactoring, which is not mentioned in the summary but seems appropriate.574-589: The
postViaAxiosfunction has been updated with documentation comments and potential refactoring, which is not mentioned in the summary but seems appropriate.packages/runtime/client/rest/src/contract/guards.ts (2)
25-25: The export
RolesConfigurationGuardsuggests that it is a guard forRolesConfiguration. Verify that this aligns with the newRolesConfigurationentity and that the guard is correctly implemented and used whereverRolesConfigurationis expected.16-27: Verify that the new guards such as
PolicyIdGuard,RoleNameGuard,RoleName, andStakeAddressBech32mentioned in the summary are defined and exported correctly in the appropriate modules, as they are not visible in the provided hunk.packages/runtime/client/rest/src/contract/index.ts (1)
- 14-54: The changes to the exported entities align with the PR objectives and summaries provided, indicating a refactor in contract creation and role configuration. Ensure that all dependent modules and external code that import these entities are updated to reflect these changes.
The verification process confirms that the changes made to the exported entities are consistent with the PR objectives and summaries. The removed entities
RolesConfig,CreateContractRequest, andCreateContractResponseare no longer present in the updated files, and the new entitiesBuildCreateContractTxRequest,BuildCreateContractTxResponse, andRolesConfigurationare being used in the codebase as intended.packages/runtime/lifecycle/src/api.ts (5)
- 37-41: The summary indicates that the
contractproperty inCreateContractRequestshould be changed tocontractOrSourceId, but the hunk still showscontract. Please verify if the property name should be updated.- contract: Contract; + contractOrSourceId: ContractOrSourceId;
47-209: The summary does not mention the addition of
stakeAddress,threadRoleName,roles,tags, andmetadataproperties toCreateContractRequest, but they are present in the hunk. This seems to be an oversight in the summary.59-145: The extensive documentation on the roles configuration in
CreateContractRequestis a significant addition that is not mentioned in the summary.150-207: The detailed information about Marlowe Tags and Cardano Metadata in
CreateContractRequestis a significant addition that is not mentioned in the summary.212-212: The summary does not mention the addition of
invalidBeforeandinvalidHereafterproperties toApplyInputsRequest, but they are present in the hunk. This seems to be an oversight in the summary.packages/runtime/lifecycle/src/generic/contracts.ts (6)
7-10: The summary indicates that the
minUTxODepositDefaultimport was removed, but this change is not visible in the provided hunks. Please verify if this change occurred outside the provided hunks.60-71: The summary does not mention changes to the
submitApplyInputsTxfunction, but the hunk shows modifications in its implementation. Please verify if these changes are intentional and if they should be documented in the summary.83-97: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [73-91]
The summary does not mention changes to the
getApplicableInputsfunction, but the hunk shows modifications in its implementation. Please verify if these changes are intentional and if they should be documented in the summary.
- 102-110: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [93-110]
The summary does not mention changes to the
getContractIdsfunction, but the hunk shows modifications in its implementation. Please verify if these changes are intentional and if they should be documented in the summary.
139-139: The summary does not mention changes to the
getPartiesfunction, but the hunk shows modifications in its implementation. Please verify if these changes are intentional and if they should be documented in the summary.149-196: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [141-190]
The summary does not mention changes to the
submitCreateTxFpTsfunction, but the hunk shows modifications in its implementation. Please verify if these changes are intentional and if they should be documented in the summary.packages/runtime/lifecycle/src/generic/payouts.ts (4)
27-35: The changes to the
mkPayoutLifecyclefunction, including the addition of therestClientparameter and renaming ofresttodeprecatedRestAPI, are consistent with the PR's objectives to refactor the REST API client interface.40-64: The updates to the function signatures for
fetchAvailablePayouts,withdrawPayouts, andfetchWithdrawnPayoutsto usedeprecatedRestAPIare consistent with the PR's objectives to refactor the REST API client interface.173-181: The modifications to the
convertAssetandconvertTokensfunctions to use types fromRestPayoutare consistent with the PR's objectives to refactor the payout processes.170-171: Consider throwing an
Errorobject instead of a string for better error handling and stack tracing.- throw `Rest API Inconsistencies for Payout API (payout withdrawn without a withdrawalID) : ${stringify(payoutDetails)}`; + throw new Error(`Rest API Inconsistencies for Payout API (payout withdrawn without a withdrawalID) : ${stringify(payoutDetails)}`);packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (3)
13-26: The addition of new imports
unAddressBech32,mintRole,openRole,AddressBech32, andAddressBech32Guardis consistent with the changes described in the summary.67-68: The
createContractmethod still uses thecontractparameter instead of the updatedcontractOrSourceIdas mentioned in the summary. This needs to be verified and potentially corrected to align with the intended changes.70-75: The usage of
mintRoleto assign roles toswapRequest.provider.roleNameandswapRequest.swapper.roleNamealigns with the changes described in the summary.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- changelog.d/20231208_095010_nicolas.henin.md (1 hunks)
Additional comments: 2
changelog.d/20231208_095010_nicolas.henin.md (2)
3-4: The changelog correctly documents the renaming of the
createContractendpoint tobuildCreateContractTxand its parity with the REST API request structure.8-8: The changelog entry for
createContractbeing complete request-wise for creating non-merkleized contracts aligns with the PR's objectives to enhance the contract creation process.
nhenin
dismissed
hrajchert’s stale review
Already multiple rounds of reviews have been done.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- packages/runtime/client/rest/package.json
Files selected for processing (31)
- changelog.d/20231208_095010_nicolas.henin.md (1 hunks)
- examples/rest-client-flow/index.html (1 hunks)
- examples/run-lite/index.html (1 hunks)
- examples/survey-workshop/participant/index.js (1 hunks)
- jsdelivr-npm-importmap.js (1 hunks)
- packages/adapter/src/codec.ts (1 hunks)
- packages/language/core/v1/src/address.ts (1 hunks)
- packages/language/core/v1/src/guards.ts (2 hunks)
- packages/language/core/v1/src/index.ts (1 hunks)
- packages/language/core/v1/src/participants.ts (1 hunks)
- packages/language/core/v1/src/policyId.ts (1 hunks)
- packages/language/core/v1/src/token.ts (2 hunks)
- packages/runtime/client/rest/src/contract/details.ts (2 hunks)
- packages/runtime/client/rest/src/contract/endpoints/collection.ts (8 hunks)
- packages/runtime/client/rest/src/contract/guards.ts (1 hunks)
- packages/runtime/client/rest/src/contract/header.ts (2 hunks)
- packages/runtime/client/rest/src/contract/index.ts (1 hunks)
- packages/runtime/client/rest/src/contract/rolesConfigurations.ts (1 hunks)
- packages/runtime/client/rest/src/index.ts (9 hunks)
- packages/runtime/core/src/address.ts (2 hunks)
- packages/runtime/core/src/index.ts (1 hunks)
- packages/runtime/lifecycle/src/api.ts (2 hunks)
- packages/runtime/lifecycle/src/browser/index.ts (2 hunks)
- packages/runtime/lifecycle/src/generic/contracts.ts (7 hunks)
- packages/runtime/lifecycle/src/generic/payouts.ts (3 hunks)
- packages/runtime/lifecycle/src/generic/runtime.ts (1 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/runtime/lifecycle/test/generic/payouts.e2e.spec.ts (2 hunks)
- packages/wallet/src/browser/index.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- changelog.d/20231208_095010_nicolas.henin.md
Files skipped from review as they are similar to previous changes (24)
- examples/rest-client-flow/index.html
- examples/run-lite/index.html
- examples/survey-workshop/participant/index.js
- jsdelivr-npm-importmap.js
- packages/adapter/src/codec.ts
- packages/language/core/v1/src/address.ts
- packages/language/core/v1/src/guards.ts
- packages/language/core/v1/src/index.ts
- packages/language/core/v1/src/participants.ts
- packages/language/core/v1/src/policyId.ts
- packages/language/core/v1/src/token.ts
- packages/runtime/client/rest/src/contract/details.ts
- packages/runtime/client/rest/src/contract/guards.ts
- packages/runtime/client/rest/src/contract/header.ts
- packages/runtime/client/rest/src/contract/index.ts
- packages/runtime/client/rest/src/index.ts
- packages/runtime/core/src/address.ts
- packages/runtime/core/src/index.ts
- packages/runtime/lifecycle/src/api.ts
- packages/runtime/lifecycle/src/browser/index.ts
- packages/runtime/lifecycle/src/generic/runtime.ts
- packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts
- packages/runtime/lifecycle/test/generic/payouts.e2e.spec.ts
- packages/wallet/src/browser/index.ts
Additional comments: 19
packages/runtime/client/rest/src/contract/endpoints/collection.ts (7)
11-17: The import of
RoleNamefrom@marlowe.io/language-core-v1is consistent with the summary.43-45: The renaming of
RolesConfigtoRolesConfigurationand the update of its import path is consistent with the summary.205-214: The addition of
ContractOrSourceIdandContractOrSourceIdGuardis present and consistent with the summary.534-537: The renaming of
CreateContractResponsetoBuildCreateContractTxResponseis not directly visible in the provided code. However, the presence ofBuildCreateContractTxResponseinterface suggests that the renaming has taken place.574-589: The type of
postViaAxioshas been updated toBuildCreateContractTxEndpoint, which is consistent with the summary.211-214: The documentation for
ContractOrSourceIdGuardshould follow the established style mentioned in the previous comments by hrajchert. Verify if the documentation style for guards is consistent across the codebase.
The review comment requested verification of the documentation style for
ContractOrSourceIdGuardto ensure consistency across the codebase. The shell command was executed to search for the guard documentation style, and it returned a single instance where the guard forContractOrSourceIdtype is documented.Since only one instance was found, it suggests that there may not be a well-established style for guard documentation across the codebase, or that the style is not consistently applied. However, without further instances to compare, we cannot definitively conclude that the style is inconsistent.
Final review comment:
The documentation for `ContractOrSourceIdGuard` is present, but we only found one instance of guard documentation in the codebase. It's recommended to ensure that the documentation style for guards is consistent across the codebase if there are other guards present.
- 517-528: The
PostContractsRequesttype has been updated with new fields and guards, which is consistent with the changes mentioned in the summary. However, the summary does not mention the addition ofthreadTokenNameandminUTxODepositfields. Verify if these additions are intentional and correctly implemented.packages/runtime/client/rest/src/contract/rolesConfigurations.ts (3)
39-46: Consider consolidating the
OpenRoletype and theopenRoleconstant into a single entity to avoid duplication and potential inconsistencies.210-216: Ensure that the
useMintedRolesfunction is used correctly throughout the codebase, especially considering the changes in role configuration.226-239: Ensure that the
mintRolefunction is used correctly throughout the codebase, especially considering the changes in role configuration.packages/runtime/lifecycle/src/generic/contracts.ts (3)
60-71: The summary does not mention the logic within
submitApplyInputsTxfunction, which seems to have been updated to usedeprecatedRestAPIinstead of the newrestClient. This change should be verified to ensure it aligns with the intended use of the deprecated API.149-196: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [139-188]
The summary does not mention the logic within
getPartiesfunction, which seems to have been updated to include additional logic for filtering tokens byroleMintingPolicyIdand mapping them toPartyobjects. This change should be verified to ensure it aligns with the intended functionality.
- 149-190: The summary does not mention the detailed logic within
submitCreateTxFpTsfunction, which includes building a transaction for contract creation and submitting it. This change should be verified to ensure it aligns with the intended functionality and the newbuildCreateContractTxandsubmitContractmethods are used correctly.packages/runtime/lifecycle/src/generic/payouts.ts (4)
27-35: The addition of the
restClientparameter tomkPayoutLifecycleis consistent with the PR's objective to transition to a new REST client implementation.40-64: The update to use
deprecatedRestAPIinfetchAvailablePayouts,withdrawPayouts, andfetchWithdrawnPayoutsfunctions aligns with the PR's objective to maintain compatibility with both the deprecated and new REST API implementations.173-181: The modifications in
convertAssetandconvertTokensfunctions to use types fromRestPayoutare consistent with the PR's objective to refactor and rename related entities.170-171: Consider throwing an
Errorobject instead of a string for better error handling and stack tracing.- throw `Rest API Inconsistencies for Payout API (payout withdrawn without a withdrawalID) : ${stringify(payoutDetails)}`; + throw new Error(`Rest API Inconsistencies for Payout API (payout withdrawn without a withdrawalID) : ${stringify(payoutDetails)}`);packages/runtime/lifecycle/src/index.ts (1)
- 42-47: The changes to
mkRuntimeLifecyclefunction are consistent with the PR objectives and the provided summary. The introduction of a new REST client (restClient) alongside the deprecated one (deprecatedRestAPI) is clear and matches the updated function signature inGeneric.mkRuntimeLifecycle. Ensure that the rest of the codebase and any documentation are updated to reflect these changes.packages/runtime/lifecycle/src/nodejs/index.ts (1)
- 22-24: The function now initializes two REST clients,
deprecatedRestAPIandrestClient, to support both the old and new REST API implementations during the transition phase. Ensure that downstream code that usesmkRuntimeLifecycleis updated to handle both REST client instances appropriately.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- packages/runtime/client/rest/package.json
Files selected for processing (32)
- changelog.d/20231208_095010_nicolas.henin.md (1 hunks)
- examples/rest-client-flow/index.html (1 hunks)
- examples/run-lite/index.html (1 hunks)
- examples/survey-workshop/participant/index.js (1 hunks)
- jsdelivr-npm-importmap.js (1 hunks)
- packages/adapter/src/codec.ts (1 hunks)
- packages/language/core/v1/src/address.ts (1 hunks)
- packages/language/core/v1/src/guards.ts (2 hunks)
- packages/language/core/v1/src/index.ts (1 hunks)
- packages/language/core/v1/src/participants.ts (1 hunks)
- packages/language/core/v1/src/policyId.ts (1 hunks)
- packages/language/core/v1/src/token.ts (2 hunks)
- packages/runtime/client/rest/src/contract/details.ts (2 hunks)
- packages/runtime/client/rest/src/contract/endpoints/collection.ts (8 hunks)
- packages/runtime/client/rest/src/contract/guards.ts (1 hunks)
- packages/runtime/client/rest/src/contract/header.ts (2 hunks)
- packages/runtime/client/rest/src/contract/index.ts (1 hunks)
- packages/runtime/client/rest/src/contract/rolesConfigurations.ts (1 hunks)
- packages/runtime/client/rest/src/index.ts (9 hunks)
- packages/runtime/core/src/address.ts (2 hunks)
- packages/runtime/core/src/index.ts (1 hunks)
- packages/runtime/core/src/sourceId.ts (1 hunks)
- packages/runtime/lifecycle/src/api.ts (2 hunks)
- packages/runtime/lifecycle/src/browser/index.ts (2 hunks)
- packages/runtime/lifecycle/src/generic/contracts.ts (7 hunks)
- packages/runtime/lifecycle/src/generic/payouts.ts (3 hunks)
- packages/runtime/lifecycle/src/generic/runtime.ts (1 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/runtime/lifecycle/test/generic/payouts.e2e.spec.ts (2 hunks)
- packages/wallet/src/browser/index.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- jsdelivr-npm-importmap.js
Files skipped from review as they are similar to previous changes (22)
- changelog.d/20231208_095010_nicolas.henin.md
- examples/rest-client-flow/index.html
- examples/run-lite/index.html
- examples/survey-workshop/participant/index.js
- packages/adapter/src/codec.ts
- packages/language/core/v1/src/address.ts
- packages/language/core/v1/src/guards.ts
- packages/language/core/v1/src/index.ts
- packages/language/core/v1/src/participants.ts
- packages/language/core/v1/src/policyId.ts
- packages/language/core/v1/src/token.ts
- packages/runtime/client/rest/src/contract/details.ts
- packages/runtime/client/rest/src/contract/guards.ts
- packages/runtime/client/rest/src/contract/header.ts
- packages/runtime/core/src/address.ts
- packages/runtime/core/src/index.ts
- packages/runtime/lifecycle/src/api.ts
- packages/runtime/lifecycle/src/browser/index.ts
- packages/runtime/lifecycle/src/generic/runtime.ts
- packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts
- packages/runtime/lifecycle/test/generic/payouts.e2e.spec.ts
- packages/wallet/src/browser/index.ts
Additional comments: 30
packages/runtime/client/rest/src/contract/endpoints/collection.ts (10)
11-17: The import of
RoleNamefrom@marlowe.io/language-core-v1aligns with the PR's objective to enhance role configuration management.41-45: Renaming
RolesConfigtoRolesConfigurationand updating the import path is consistent with the PR's objective to refactor role-related entities.205-214: The addition of
ContractOrSourceIdandContractOrSourceIdGuardis consistent with the PR's objective to improve the contract creation process.555-561: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [534-561]
The update of
CreateContractResponsetoBuildCreateContractTxResponseand the modification of its properties and comments align with the PR's objective to transition to a new REST client implementation.
574-589: The update of the type of
postViaAxiostoBuildCreateContractTxEndpointis consistent with the PR's objective to refactor the contract creation endpoint.66-67: The changes to
GetContractsRequestare not mentioned in the summary. Please ensure that these changes are intentional and consistent with the PR's objectives.168-168: The changes to
GetContractsResponseare not mentioned in the summary. Please ensure that these changes are intentional and consistent with the PR's objectives.286-286: The changes to
BuildCreateContractTxRequestOptionsare not mentioned in the summary. Please ensure that these changes are intentional and consistent with the PR's objectives.518-518: The changes to
PostContractsRequestare not mentioned in the summary. Please ensure that these changes are intentional and consistent with the PR's objectives.534-534: The changes to
BuildCreateContractTxResponseare not mentioned in the summary. Please ensure that these changes are intentional and consistent with the PR's objectives.packages/runtime/client/rest/src/contract/index.ts (1)
- 14-54: The changes to the exported entities are consistent with the PR objectives and the provided summary. The removal of
RolesConfigand the addition of new entities fromrolesConfigurations.js, as well as the renaming of contract-related entities, are correctly reflected in the exports.packages/runtime/client/rest/src/index.ts (7)
80-82: The renaming of
createContracttobuildCreateContractTxaligns with the PR's objective to shift functionality from creating a contract directly to building a transaction for contract creation.454-454: The renaming of
RestDIto includedeprecatedRestAPIandrestClientis consistent with the PR's objective to transition to a new REST client implementation.411-411: The update of the
ContractsAPIinterface with the newposttypeContracts.BuildCreateContractTxEndpointis consistent with the renaming of thecreateContractfunction.235-255: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [238-263]
The implementation of
buildCreateContractTxhas been updated to reflect the new request structure, which is consistent with the PR's objective to enhance the contract creation process.
123-125: The previous comments regarding the potential renaming and documentation updates within the
RestClientinterface have been addressed in the current code.238-238: The previous comment by hrajchert regarding the name change in the backend for future release to maintain 1-1 parity has been addressed in the current code.
240-240: The previous comment by bjornkihlberg regarding the clean and simple idiomatic TypeScript has been addressed in the current code.
packages/runtime/core/src/sourceId.ts (1)
- 3-4: The changes to
SourceIdandSourceIdGuardare straightforward and do not introduce any issues.packages/runtime/lifecycle/src/generic/contracts.ts (6)
7-10: The summary indicates that the
minUTxODepositDefaultimport was removed, but this is not reflected in the provided hunk. Please verify if this change is present elsewhere in the file or if the summary is incorrect.19-25: The addition of
transactionWitnessSetTextEnvelopeandRestClientimports aligns with the PR objectives to enhance contract creation and REST client implementation.36-44: The
mkContractLifecyclefunction's signature has been correctly updated to include therestClientparameter, aligning with the PR's goal of transitioning to a new REST client implementation.50-53: The renaming of the
restparameter torestClientin thesubmitCreateTxfunction is consistent with the PR's refactoring goals.141-144: The
submitCreateTxFpTsfunction's signature has been correctly updated to take aRestClienttype, aligning with the PR's goal of transitioning to a new REST client implementation.149-189: The changes within
submitCreateTxFpTsfunction to useclient.buildCreateContractTxandclient.submitContractare in line with the PR's objectives to refactor contract creation into a transaction-building process.packages/runtime/lifecycle/src/generic/payouts.ts (3)
27-35: The changes to the
mkPayoutLifecyclefunction align with the PR objectives and summaries, indicating a transition to a new REST client implementation.40-64: The updates to the
fetchAvailablePayouts,withdrawPayouts, andfetchWithdrawnPayoutsfunctions to usedeprecatedRestAPIare in line with the PR objectives and summaries.173-181: The modifications to the
convertAssetandconvertTokensfunctions to useRestPayouttypes are consistent with the PR objectives and summaries.packages/runtime/lifecycle/src/index.ts (1)
- 17-23: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [17-47]
The changes to
mkRuntimeLifecyclefunction correctly reflect the PR's objective to support both the deprecated and new REST clients. The function now initializes two REST clients and passes them toGeneric.mkRuntimeLifecyclealong with thewalletparameter, which aligns with the PR's goal of transitioning to a new REST client implementation.packages/runtime/lifecycle/src/nodejs/index.ts (1)
- 8-24: The changes to
mkRuntimeLifecyclefunction correctly implement the transition to a new REST client alongside the deprecated one, as per the PR objectives. The function now initializes bothdeprecatedRestAPIandrestClientwith the sameruntimeURLand passes them along with thewallettoGeneric.mkRuntimeLifecycle. This is consistent with the intended transition phase where both REST API implementations are in use.
| ) | ||
| ); |
There was a problem hiding this comment.
Consider throwing an Error object instead of a string for better error handling and stack tracing.
- throw `Rest API Inconsistencies for Payout API (payout withdrawn without a withdrawalID) : ${stringify(payoutDetails)}`;
+ throw new Error(`Rest API Inconsistencies for Payout API (payout withdrawn without a withdrawalID) : ${stringify(payoutDetails)}`);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.
| ) | |
| ); | |
| throw new Error(`Rest API Inconsistencies for Payout API (payout withdrawn without a withdrawalID) : ${stringify(payoutDetails)}`); |
| // TODO: Jamie, remove the `s from the end of the endpoint name in the docs site | ||
| // DISCUSSION: @Jamie, @N.H: Should this be called `buildApplyInputsToContractTx` instead? As it is not applying inputs to the | ||
| // contract, rather it is creating the transaction to be signed |
There was a problem hiding this comment.
The TODO comments and discussions within the code should be resolved or removed to avoid confusion in the final codebase.
| // TODO: Jamie, remove the `s from the end of the endpoint name in the docs site | ||
| // DISCUSSION: @Jamie, @N.H: Should this be called `buildWithdrawPayoutsTx` instead? As it is not withdrawing the | ||
| // payout, rather it is creating the transaction to be signed |
There was a problem hiding this comment.
The TODO comments and discussions within the code should be resolved or removed to avoid confusion in the final codebase.
This PR fulfills 2 issues :
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Refactor
Style
Documentation
Tests
Chores