PLT-9089: create contract bundle support #167
Conversation
WalkthroughThe updates reflect a shift towards a more modular and testable codebase, with changes to configuration reading and endpoint testing. A new client creation pattern is introduced for REST interactions, and contract creation logic is expanded to handle multiple request types. The testing infrastructure is notably enhanced with better configuration handling and project root detection, streamlining the development and testing workflow. 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: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- tsconfig.test.json
Files selected for processing (11)
- .vscode/settings.json (1 hunks)
- doc/howToDevelop.md (1 hunks)
- examples/nodejs/src/marlowe-object-flow.ts (1 hunks)
- packages/runtime/client/rest/test/endpoints/contracts.spec.e2e.ts (5 hunks)
- packages/runtime/client/rest/test/endpoints/payouts.spec.e2e.ts (4 hunks)
- packages/runtime/lifecycle/src/api.ts (4 hunks)
- packages/runtime/lifecycle/src/generic/contracts.ts (5 hunks)
- packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (3 hunks)
- packages/runtime/lifecycle/test/generic/contracts.e2e.spec.ts (3 hunks)
- packages/testing-kit/src/environment/configuration.ts (1 hunks)
- packages/testing-kit/src/environment/index.ts (4 hunks)
Additional comments: 11
.vscode/settings.json (1)
- 15-20: The updates to include additional paths for test distributions in
.vscode/settings.jsonlook good and are consistent with the project structure.packages/runtime/client/rest/test/endpoints/payouts.spec.e2e.ts (1)
- 11-12: The refactoring to use
readTestConfigurationandmkRestClientinpayouts.spec.e2e.tsis correctly implemented.packages/testing-kit/src/environment/configuration.ts (1)
- 24-61: The refactoring in
configuration.tsto use io-ts for configuration validation and the introduction ofreadTestConfigurationfunction are correctly implemented.packages/runtime/client/rest/test/endpoints/contracts.spec.e2e.ts (1)
- 13-14: The refactoring to use
readTestConfigurationandmkRestClientincontracts.spec.e2e.tsis correctly implemented.packages/testing-kit/src/environment/index.ts (1)
- 78-95: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-93]
The updates in
index.tsto align with the new testing and configuration approach, including the use ofmkRuntimeLifecycleandmkRestClient, are correctly implemented.packages/runtime/lifecycle/test/generic/contracts.e2e.spec.ts (1)
- 27-30: The refactoring to use
readTestConfigurationandmkLifecycleincontracts.e2e.spec.tsis correctly implemented.packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (1)
- 36-37: The refactoring to use
readTestConfigurationandmkLifecycleinswap.ada.token.e2e.spec.tsis correctly implemented.doc/howToDevelop.md (1)
- 62-76: The documentation in
howToDevelop.mdhas been updated to reflect the new JSON file format for environment configuration, which is correctly detailed.packages/runtime/lifecycle/src/generic/contracts.ts (1)
- 104-156: The updates in
contracts.ts, including the addition ofCreateContractRequestBase,BuildCreateContractTxRequestOptions, and the expandedcreateContractfunction, are correctly implemented.packages/runtime/lifecycle/src/api.ts (1)
- 82-174: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [24-235]
The updates in
api.ts, including the addition ofContractBundle, new interfacesCreateContractRequestFromContractandCreateContractRequestFromBundle, and the union typeCreateContractRequest, are correctly implemented.examples/nodejs/src/marlowe-object-flow.ts (1)
- 570-573: The
createContractfunction has been updated to include aminimumLovelaceUTxODepositparameter when callinglifecycle.contracts.createContract. This is likely related to the PR's objective to support contract bundles. Ensure that the value of3_000_000lovelaces is appropriate and consistent with the rest of the application's logic and requirements.Verification successful
The search results confirm that the
minimumLovelaceUTxODepositparameter is consistently used across the application with the value3_000_000. This consistency is found in test specifications, example code, and within the contract creation logic. There are no indications of inconsistencies or deviations from this value.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the minimumLovelaceUTxODeposit is consistent across the application rg 'minimumLovelaceUTxODeposit' --type tsLength of output: 1572
| | CreateContractRequestFromBundle; | ||
|
|
||
| export interface CreateContractRequestFromContract | ||
| extends CreateContractRequestBase { |
There was a problem hiding this comment.
Not sure, this pattern is better than a ContractOrSourceId parameter, I'd prefer also Composition over Inheritance ... and the doc is probably less readable that way... but ok, we can always change it later when we'll add more functionalities...
There was a problem hiding this comment.
take a look at the generated docs, starting from the function. I think it ended up being readable.
IMO, Composition vs Inheritance does not apply to this. I'm not doing class inheritance
| */ | ||
| export type TestConfiguration = { | ||
| bank: { seedPhrase: SeedPhrase }; | ||
| lucid: { blockfrost: Blockfrost; node: { network: Lucid.Network } }; |
There was a problem hiding this comment.
You've re-designed this TestConfiguration type without really using it and discuss with me before... It will impact my current work... Do we proceed anyway or I'll handle the changes myself ?
There was a problem hiding this comment.
This was a first attempt to settup debugging in the tests and I managed to do that. I've barely modified the "testing infrastructure". Do any change you want on top, make sure it is debuggable still ;). I have made sure that the e2e tests pass before and after my changes
| const status = await runtimeClient.healthcheck(); | ||
| const runtimeNodeNetwork = getNetwork(status.networkId); | ||
|
|
||
| if (!G.CompatibleRuntimeVersion.is(status.version)) { |
There was a problem hiding this comment.
Where do you provide information on this ?
There was a problem hiding this comment.
In the e2e test I don't
| blockfrostProjectId: t.string, | ||
| blockfrostUrl: t.string, | ||
| }), | ||
| network: lucidNetworkGuard, |
There was a problem hiding this comment.
We don't want necessary to have a lucid terminology exposed in our config file for the network... and this wasn't done yet , but we should check if the networks are compatible as well
There was a problem hiding this comment.
it is a union type of the words "Preprod", "Preview" and "Mainnet". I don't think it applies to "lucid terminology". And in any case it is used to setup a lucid wallet.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- changelog.d/20240112_130130_hrajchert_plt_9089_createContract_bundle_support.md (1 hunks)
Additional comments: 1
changelog.d/20240112_130130_hrajchert_plt_9089_createContract_bundle_support.md (1)
- 2-5: The changelog entry is clear and includes a reference to the PR, which is good practice for traceability.
hrajchert
force-pushed
the
hrajchert/plt-9089-createContract-bundle-support
branch
from
34ed0f0 to
f870768
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
tsconfig.test.jsonis excluded by:!**/*.json
Files selected for processing (12)
- .vscode/settings.json (1 hunks)
- changelog.d/20240112_130130_hrajchert_plt_9089_createContract_bundle_support.md (1 hunks)
- doc/howToDevelop.md (1 hunks)
- examples/nodejs/src/marlowe-object-flow.ts (1 hunks)
- packages/runtime/client/rest/test/endpoints/contracts.spec.e2e.ts (5 hunks)
- packages/runtime/client/rest/test/endpoints/payouts.spec.e2e.ts (4 hunks)
- packages/runtime/lifecycle/src/api.ts (4 hunks)
- packages/runtime/lifecycle/src/generic/contracts.ts (5 hunks)
- packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (9 hunks)
- packages/runtime/lifecycle/test/generic/contracts.e2e.spec.ts (3 hunks)
- packages/testing-kit/src/environment/configuration.ts (1 hunks)
- packages/testing-kit/src/environment/index.ts (4 hunks)
Files skipped from review as they are similar to previous changes (10)
- .vscode/settings.json
- changelog.d/20240112_130130_hrajchert_plt_9089_createContract_bundle_support.md
- doc/howToDevelop.md
- packages/runtime/client/rest/test/endpoints/contracts.spec.e2e.ts
- packages/runtime/client/rest/test/endpoints/payouts.spec.e2e.ts
- packages/runtime/lifecycle/src/api.ts
- packages/runtime/lifecycle/src/generic/contracts.ts
- packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts
- packages/runtime/lifecycle/test/generic/contracts.e2e.spec.ts
- packages/testing-kit/src/environment/index.ts
Additional comments: 5
packages/testing-kit/src/environment/configuration.ts (4)
- 5-10: The
lucidNetworkGuardunion type is correctly defined to include literals for network types.- 12-20: The
testConfigurationGuardio-ts type is well-defined and matches the expected structure for test configurations.- 24-41: The
findRootDirfunction is correctly implemented to recursively find the root directory by checking for the existence oftsconfig-base.json.- 44-44: It's important to ensure that the
rootDirconstant is correctly set in all environments where the tests might run.Verification successful
The corrected script has successfully located the
tsconfig-base.jsonfile within the directory structure of the repository. This confirms that the file exists and suggests that thefindRootDirfunction, which likely relies on the presence of this file to determine the project's root directory, should operate correctly in this environment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that tsconfig-base.json exists in the expected directory structure. fd --type f --name "tsconfig-base.json"Length of output: 290
Script:
#!/bin/bash # Correcting the command to verify that tsconfig-base.json exists in the expected directory structure. fd --type f "tsconfig-base.json"Length of output: 51
examples/nodejs/src/marlowe-object-flow.ts (1)
- 570-573: The refactoring of the
createContractfunction to uselifecycle.contracts.createContractis correctly implemented and simplifies the contract creation process.
| export async function readTestConfiguration( | ||
| filepath?: string | ||
| ): Promise<TestConfiguration> { | ||
| if (!filepath) { | ||
| filepath = `${rootDir}/env/.test-env.json`; | ||
| } | ||
| const configStr = await readFile(filepath, { encoding: "utf-8" }); | ||
| const result = testConfigurationGuard.decode(JSON.parse(configStr)); | ||
| if (result._tag === "Left") { | ||
| throw new Error("Invalid configuration"); | ||
| } | ||
| }; | ||
| return result.right; | ||
| } |
There was a problem hiding this comment.
The testConfigurationGuard is implemented using io-ts, which is capable of providing detailed error messages. However, the current error handling in the readTestConfiguration function throws a generic error without these details. To improve debugging, the error message should include the specific validation failures from io-ts.
Analysis chain
The readTestConfiguration function correctly decodes the configuration file using io-ts. However, it's important to verify that the error message for an invalid configuration is informative enough for debugging purposes.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the error message for an invalid configuration is clear and actionable.
# This would involve checking the error handling behavior when an invalid configuration is provided.
# Since this is a hypothetical verification, no actual script is provided.
Length of output: 0
Script:
#!/bin/bash
# Let's find out where the `testConfigurationGuard` is defined and how it's implemented.
# This will help us understand what kind of error messages it produces when decoding fails.
# First, we search for the definition of `testConfigurationGuard`.
rg "const testConfigurationGuard"
Length of output: 136
Script:
#!/bin/bash
# Additionally, we should check if there are any custom error handling or reporting mechanisms
# that are used in conjunction with `testConfigurationGuard.decode`.
# Search for any custom error handling related to `testConfigurationGuard.decode`.
rg "testConfigurationGuard.decode" -A 10
Length of output: 542
Script:
#!/bin/bash
# Now we need to inspect the implementation of `testConfigurationGuard` to see if it provides
# detailed error messages that would be included when the decoding fails.
# Since we know the file where `testConfigurationGuard` is defined, let's look at its implementation.
cat packages/testing-kit/src/environment/configuration.ts | grep -A 20 "const testConfigurationGuard"
Length of output: 746
hrajchert
deleted the
hrajchert/plt-9089-createContract-bundle-support
branch
This PR modifies the
runtimeLifecycle.contracts.createContractmethod to support bundles in a non-breaking way.Additionaly I also modified the e2e test configuration to be able to debug it.
For the moment to debug e2e tests you need to build (recommended with watch mode) in one terminal
Open up a VSCode Debug Terminal and execute
That should attach the debugging session correctly with the source maps. In a future PR I'll modify the launch configuration to simplify this even further.
Summary by CodeRabbit
New Features
Improvements
Refactor
Documentation
Bug Fixes