test(test-suite): enable withdrawal tests #2202
Conversation
WalkthroughThe pull request introduces changes to the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
shumkov
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/platform-test-suite/test/e2e/withdrawals.spec.js (1)
Line range hint
206-240: Acknowledged: Skipped test for direct withdrawal document creation.The test for preventing direct creation of withdrawal documents is currently skipped. This is appropriate given the TODO comment explaining that a consensus error needs to be implemented to replace the current client-side validation.
To ensure this improvement is not forgotten, would you like me to create a GitHub issue to track the implementation of the consensus error for this case? This would help in following up on this TODO and ensuring that this important security check is properly implemented in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/js-dash-sdk/src/SDK/Client/Platform/methods/identities/creditWithdrawal.ts (1 hunks)
- packages/platform-test-suite/test/e2e/withdrawals.spec.js (7 hunks)
🔇 Additional comments (7)
packages/js-dash-sdk/src/SDK/Client/Platform/methods/identities/creditWithdrawal.ts (1)
53-53: Approve the change and verify its impact across the codebase.The reordering of the spread operator for the
optionsobject is a positive change. It now correctly allows user-provided options to override the defaultsigningKeyIndex, which aligns with the expected behavior of option objects in JavaScript.To ensure this change doesn't unexpectedly affect other parts of the codebase, please run the following script to check for any usage of
creditWithdrawalthat might be impacted:Review the results to ensure that all calls to
creditWithdrawalare still behaving as expected with this change.✅ Verification successful
To address the issue with the previous shell scripts, please run the following updated scripts to search for usages of the
creditWithdrawalfunction in TypeScript files:
Verification Completed: No usages of
creditWithdrawalfound with explicit options or impacted by thesigningKeyIndexchange.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of creditWithdrawal function to verify impact of the options change. # Test: Look for calls to creditWithdrawal with explicit options rg --type typescript -A 5 'creditWithdrawal\s*\([^)]*,\s*[^)]*,\s*\{[^}]*\}' # Test: Look for calls to creditWithdrawal that might be affected by the default signingKeyIndex rg --type typescript -A 5 'creditWithdrawal\s*\([^)]*\)'Length of output: 207
Script:
#!/bin/bash # Description: Search for usages of creditWithdrawal function to verify impact of the options change. # Test: Look for calls to creditWithdrawal with explicit options rg 'creditWithdrawal\s*\([^)]*,\s*[^)]*,\s*\{[^}]*\}' -g '*.ts' -g '*.tsx' -A 5 # Test: Look for calls to creditWithdrawal that might be affected by the default signingKeyIndex rg 'creditWithdrawal\s*\([^)]*\)' -g '*.ts' -g '*.tsx' -A 5Length of output: 143
packages/platform-test-suite/test/e2e/withdrawals.spec.js (6)
11-11: LGTM: Test suite re-enabled as intended.The re-enabling of the "Withdrawals" test suite aligns with the PR objective. This is a positive step towards improving test coverage for the withdrawal functionality.
Consider running these tests multiple times to ensure they are no longer flaky. If flakiness persists, it might be worth investigating and addressing the root causes before merging.
41-41: Approved: Initial balance increased. Please clarify the reason.The initial balance for identities in the test suite has been doubled from 1,000,000 to 2,000,000. This change appears to be intentional and likely necessary for the tests to pass.
Could you please provide some context on why this increase was necessary? It would be helpful to understand if this change is related to recent updates in the SDK or if it addresses any specific issues encountered during testing.
183-183: Approved: Improved error message clarity.The updated error message now includes the specific values for the withdrawal amount and identity balance. This enhancement provides more detailed information, which is valuable for debugging and understanding test failures.
196-198: LGTM: Added test for incorrect signing key type.The test now includes a check for using the wrong security key type during withdrawal. This is a valuable addition to ensure that only the appropriate keys (CRITICAL level) can be used for withdrawals.
Consider adding additional test cases to cover:
- A successful withdrawal using the correct key type.
- Attempts to use other invalid key types or indices.
These additional tests would further strengthen the security checks around the withdrawal process.
Line range hint
1-300: Overall assessment: Changes align with PR objectives and improve test coverage.The modifications in this file successfully re-enable the withdrawal tests and update the code to match recent SDK changes. The test cases cover various scenarios including successful withdrawals, balance checks, and error conditions.
Some suggestions for further improvement:
- Verify the reason for increasing the initial balance.
- Consider adding more test cases for different key types in withdrawals.
- Follow up on the TODO for implementing a consensus error for direct withdrawal document creation.
These changes contribute positively to the robustness of the withdrawal functionality testing.
60-62: LGTM: Updated withdrawal address handling.The change in how the withdrawal address is passed to the
withdrawCreditsmethod (now using an object with atoAddressproperty) is consistent throughout the file and aligns with recent SDK changes mentioned in the PR description.To ensure this change is applied consistently across the entire codebase, you may want to run the following command:
This will help identify any other files that might need similar updates.
Also applies to: 130-132, 180-182, 196-196, 236-238
✅ Verification successful
Additional Verification Required
The initial search did not return any matches. To ensure all
withdrawCreditscalls have been updated to use an object withtoAddress, please run the following command:
Additional Verification Required
The previous search command failed due to unsupported regex syntax. To identify any
withdrawCreditscalls that do not use an object withtoAddress, please run the following command:
Verification Successful: All
withdrawCreditscalls are updated.All instances of
withdrawCreditshave been updated to use an object with thetoAddressproperty, ensuring consistency throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of withdrawCredits that might not have been updated rg --type js 'withdrawCredits\s*\([^{]*,[^{]*,[^{]*\)' packages/Length of output: 66
Script:
#!/bin/bash # Search for withdrawCredits calls that do not use an object with `toAddress` rg --type js 'withdrawCredits\s*\(\s*(?!\{).*?\)' packages/Length of output: 322
Script:
#!/bin/bash # Search for withdrawCredits calls that do not use an object with `toAddress` rg --type js 'withdrawCredits\s*\(\s*[^{}\s]' packages/Length of output: 57
Issue being fixed or feature implemented
We would like to test withdrawals with test suite
What was done?
How Has This Been Tested?
Running test suite
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Withdrawalstest suite to ensure comprehensive testing of withdrawal functionalities.Tests