refactor: Removed sign_transaction_if_paid, and now sign_transaction …#241
Merged
dev-jodee merged 1 commit intorelease/feature-freeze-for-auditfrom Oct 28, 2025
Conversation
…and sign_and_send_transaction will always validate price, but if the node operator sets price as FREE it will have the same behavior as the old methods
📊 TypeScript Coverage ReportCoverage: 81.7% View detailed reportCoverage artifacts have been uploaded to this workflow run. |
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 24feec7 in 1 minute and 16 seconds. Click for details.
- Reviewed
3407lines of code in46files - Skipped
0files when reviewing. - Skipped posting
11draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. tests/integration.test.ts:50
- Draft comment:
Good comprehensive config checks. Consider defining constants for expected field names or error messages for more robust maintenance. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
2. tests/adversarial/fee_payer_exploitation.rs:36
- Draft comment:
Error message assertions depend on string matching. It might be more robust to assert against an error code. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. tests/free_signing/free_signing_tests.rs:20
- Draft comment:
Consider adding comments to clarify the meaning of base64 encoded transaction strings (they are used in tests for real transactions). - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
4. tests/multi_signer/signer_management.rs:72
- Draft comment:
Test for signer key consistency is well thought-out. Using explicit constants for expected signer keys might improve readability. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
5. tests/payment_address/payment_address_legacy_tests.rs:55
- Draft comment:
Tests that signTransaction fails when payment goes to wrong address use error matching on response. Consider verifying error codes if available. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
6. tests/payment_address/payment_address_multi_payment_tests.rs:109
- Draft comment:
When testing for insufficient payment, ensure error message matching is resilient; consider using error codes or defined error type instead of string contains. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
7. tests/src/common/fixtures/kora-test.toml:28
- Draft comment:
The configuration uses hard-coded public keys. Consider moving these to constants so that changes in test environments are easier to manage. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
8. tests/src/common/fixtures/kora-free-test.toml:51
- Draft comment:
The free pricing configuration is clear. Ensure that tests using this configuration explicitly document that payment validation is bypassed. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
9. tests/src/test_runner/test_cases.toml:28
- Draft comment:
Test cases are well organized. Consider adding comments for the purpose of each group, e.g. what 'initialize_payments_atas' does. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
10. tests/tokens/token_2022_extensions_test.rs:115
- Draft comment:
Ensure error string matching in blocked extension tests is robust; consider checking for specific error codes. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
11. tests/tokens/token_2022_test.rs:15
- Draft comment:
Token 2022 transfer tests properly check simulation results. The usage of TransactionUtil::decode_b64_transaction appears correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_sh2GRRylunm1iQbl
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
This file contains hidden or 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
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.
…and sign_and_send_transaction will always validate price, but if the node operator sets price as FREE it will have the same behavior as the old methods
Important
Refactor to remove
signTransactionIfPaid, updatesignTransactionto handle payment validation, and adjust tests and documentation accordingly.signTransactionIfPaidmethod and logic, replace withsignTransactionhandling payment validation.signTransactionvalidates payments unless price is FREE, then behaves like old method.free_signing_tests.rs,transaction_signing.rs, and others to usesignTransaction.free_signing_tests.rs.signTransactionIfPaid.kora.tomland other config files to removesignTransactionIfPaidreferences.signTransactionis enabled in relevant test configurations.README.mdand other docs to reflect removal ofsignTransactionIfPaid.signTransactionwith new behavior.This description was created by
for 24feec7. You can customize this summary. It will automatically update as commits are pushed.
📊 Unit Test Coverage
Unit Test Coverage: 80.4%
View Detailed Coverage Report