Integration Tests Atomic Swap Nominal Use Case (PLT-9263, part of PLT-9226) #171
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 recent updates involve tweaks to transaction handling, with a focus on the use of Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on 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 selected for processing (6)
- packages/language/examples/src/atomicSwap.ts (12 hunks)
- packages/runtime/client/rest/src/contract/rolesConfigurations.ts (1 hunks)
- packages/runtime/lifecycle/src/api.ts (1 hunks)
- packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (5 hunks)
- packages/runtime/lifecycle/test/generic/contracts.e2e.spec.ts (1 hunks)
- packages/testing-kit/src/wallet/lucid/index.ts (2 hunks)
Additional comments: 16
packages/runtime/lifecycle/test/generic/contracts.e2e.spec.ts (1)
- 22-22: The removal of
.skipenables the test case "can create a Marlowe Contract" to be executed, which is a positive change assuming the test is ready to be included in the test suite.packages/testing-kit/src/wallet/lucid/index.ts (2)
- 66-67: Adding a newline before returning from the
waitRuntimeSyncingTillCurrentWalletTipfunction improves readability of the output, and increasing the sleep duration from 5 to 15 seconds may help with synchronization issues in tests.- 83-83: Using
process.stdout.writeinstead oflogWarningfor output in theisRuntimeChainMoreAdvancedThanfunction is a minor change that does not affect functionality but may improve the output formatting.packages/runtime/client/rest/src/contract/rolesConfigurations.ts (1)
- 212-212: The comment update in the
useMintedRolesfunction clarifies the minting process for Role Tokens, making it more general and potentially more accurate.packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (6)
- 13-13: The addition of
payoutIdto the imported entities is necessary for the test to use this identifier in payout-related operations.- 26-26: The addition of
logErrorto the imported entities allows for better error logging within the test suite.- 30-39: Importing additional entities such as
onlyByContractIds,RuntimeLifecycle,Action,Closed,Scheme, andmintRoleis necessary for the expanded test coverage and utility functions at the end of the file.- 75-75: Modifying the
selleraddress assignment within theschemeobject to useawait seller.wallet.getChangeAddress()ensures that the correct address is used in the test.- 96-106: The additional logging and contract manipulation operations within the
describe("swap")block provide more detailed output and control over the contract lifecycle in the test.- 217-264: The addition of several utility functions at the end of the file, such as
getClosedState,getAvailableActions,shouldBeAClosedContract, andgetMarloweStatefromAnActiveContract, enhances the test suite's capabilities to interact with and verify the contract's state.packages/runtime/lifecycle/src/api.ts (1)
- 285-285: The added comment clarifies the expected behavior of the
Withdrawmethod in thePayoutsAPIinterface, which is to not wait for confirmation behind the scenes and to return aTxId.packages/language/examples/src/atomicSwap.ts (5)
- 72-72: The addition of the
SingleInputTximport is necessary for the updated functions that now use this type instead ofInput.- 158-168: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [152-165]
Commenting out the
RetrieveMinimumLovelaceAddedtype indicates that this action is no longer part of the atomic swap contract lifecycle, which could be due to a change in the contract's requirements or behavior.
- 211-226: Renaming the error handling classes to
UnexpectedActiveSwapContractStateandUnexpectedClosedSwapContractStateprovides clearer semantics on the type of error state encountered in the swap contract.- 259-263: Commenting out the
RetrieveMinimumLovelaceAddedaction within thegetAvailableActionsfunction aligns with the earlier comment-out of the type, maintaining consistency in the contract's logic.- 345-355: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [302-362]
The modifications to the
getState,getClosedState, andgetActiveStatefunctions to useSingleInputTxinstead ofInputare consistent with the import change and are necessary for the functions to operate correctly with the updated contract structure.
nhenin
force-pushed
the
atomicSwap/nomial-case
branch
2 times, most recently
from
d3dd96c to
8a3b78f
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- packages/language/examples/src/atomicSwap.ts (12 hunks)
- packages/runtime/client/rest/src/contract/rolesConfigurations.ts (1 hunks)
- packages/runtime/lifecycle/src/api.ts (1 hunks)
- packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (5 hunks)
- packages/runtime/lifecycle/test/generic/contracts.e2e.spec.ts (1 hunks)
- packages/testing-kit/src/wallet/lucid/index.ts (2 hunks)
Files skipped from review as they are similar to previous changes (6)
- packages/language/examples/src/atomicSwap.ts
- packages/runtime/client/rest/src/contract/rolesConfigurations.ts
- packages/runtime/lifecycle/src/api.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/wallet/lucid/index.ts
nhenin
force-pushed
the
atomicSwap/nomial-case
branch
from
8a3b78f to
9a5dc22
Compare
nhenin
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- packages/language/examples/src/atomicSwap.ts (12 hunks)
- packages/runtime/client/rest/src/contract/rolesConfigurations.ts (1 hunks)
- packages/runtime/lifecycle/src/api.ts (1 hunks)
- packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (5 hunks)
- packages/runtime/lifecycle/test/generic/contracts.e2e.spec.ts (1 hunks)
- packages/testing-kit/src/wallet/lucid/index.ts (2 hunks)
Files skipped from review as they are similar to previous changes (5)
- packages/language/examples/src/atomicSwap.ts
- packages/runtime/client/rest/src/contract/rolesConfigurations.ts
- packages/runtime/lifecycle/src/api.ts
- packages/runtime/lifecycle/test/generic/contracts.e2e.spec.ts
- packages/testing-kit/src/wallet/lucid/index.ts
Additional comments: 7
packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (7)
- 13-13: The addition of
payoutIdto the imported entities is noted and appears to be in line with the PR objectives to enhance testing for atomic swaps.- 26-26: The addition of
logErrorfunction to the imported entities is approved as it likely aids in better error logging during the integration tests.- 30-35: The addition of new imports such as
onlyByContractIds,RuntimeLifecycle,Action,Closed,Scheme, andmintRoleis approved, assuming these are necessary for the expanded testing functionality.- 71-71: The modification of the
selleraddress assignment within theschemeobject to dynamically retrieve the address is approved, as it likely ensures that the correct address is used during tests.- 92-101: The addition of logging and contract manipulation operations within the
describe("swap")block is approved, assuming these are part of the new integration tests for atomic swaps.- 110-206: The extensive changes within the
describe("swap")block, including additional logging, contract manipulation operations, and error handling, are approved. These changes seem to align with the PR's objective to enhance the testing framework.- 214-261: The addition of several utility functions at the end of the file, such as
getClosedState,getAvailableActions, andshouldBeAClosedContract, is approved. These functions likely contribute to the robustness of the integration tests by providing necessary operations for atomic swap testing.
hrajchert
approved these changes
Jan 15, 2024
| @@ -63,7 +63,8 @@ const waitRuntimeSyncingTillCurrentWalletTip = | |||
| await waitForPredicatePromise( | |||
| isRuntimeChainMoreAdvancedThan(client, currentLucidSlot) | |||
| ); | |||
| return sleep(5); | |||
| process.stdout.write("\n"); | |||
| return sleep(15); | |||
There was a problem hiding this comment.
👁️ ...
what is this sleep 5 turned into 15?
Why are we sleeping on a random number? Could you add a comment to it?
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.
This PR is part of #147, it covers only the integration tests and not the unit ones.
It fixes also #168
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Withdrawmethod in the Payouts API.Refactor
Style