-
Notifications
You must be signed in to change notification settings - Fork 207
release: Feature Freeze for Audit #223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
#221) * feat: (PRO-203) Implement TurnkeyError handling and enhance PrivySigner error logging - Added TurnkeyError enum for comprehensive error handling in TurnkeySigner. - Updated sign and create_stamp methods to return TurnkeyError variants. - Enhanced error logging in PrivySigner for API call failures, including status and response details. - Refactored TurnkeySigner initialization to remove unnecessary error handling, improving clarity. - Added tests for TurnkeyError display and conversion to KoraError. * Update coverage badge [skip ci] --------- Co-authored-by: github-actions <[email protected]>
|
Release branch to accumulate code changes until the feature freeze is done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 178fa5a in 1 minute and 45 seconds. Click for details.
- Reviewed
442lines of code in7files - Skipped
0files when reviewing. - Skipped posting
6draft 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. crates/lib/src/signer/turnkey/signer.rs:115
- Draft comment:
Avoid using unwrap() when converting signature bytes; use proper error propagation instead of try_into(). - Reason this comment was not posted:
Comment was on unchanged code.
2. crates/lib/src/signer/turnkey/signer.rs:130
- Draft comment:
Avoid unwrap() on the result of bytes_to_hex in create_stamp; propagate the error instead. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. crates/lib/src/signer/turnkey/types.rs:116
- Draft comment:
TurnkeyError conversion to KoraError uses to_string(); consider preserving more context if needed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment raises a valid point about error context preservation, it's speculative ("if needed") and doesn't provide concrete guidance. The test demonstrates the current behavior is intentional - wrapping the error message in a SigningError variant. Without seeing KoraError's definition and understanding the broader error handling strategy, we can't be certain this is actually a problem. The comment could be pointing to a legitimate architectural concern about error handling. Maybe there are cases where preserving the original error type would be valuable for error handling upstream. However, the current implementation appears intentional based on the test, and without more context about KoraError and the codebase's error handling strategy, this comment is too speculative to be actionable. The comment should be removed as it's speculative ("if needed") and requires broader context about error handling strategy to determine if it's actually an issue.
4. crates/lib/src/validator/transaction_validator.rs:49
- Draft comment:
When converting program IDs from config, consider adding more context to errors for easier debugging. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. crates/lib/src/validator/transaction_validator.rs:178
- Draft comment:
The closure 'check_if_allowed' is clear, but consider a more descriptive name (e.g. 'validate_source_account') to clarify its purpose. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. crates/lib/src/validator/transaction_validator.rs:321
- Draft comment:
Tests rely on global config changes; ensure isolation if tests run concurrently. The use of #[serial] helps, but consider using dedicated config instances. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_e6HWmCt3LneL2fyG
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
… test runner (#222) * chore: (PRO-271) Update / cleanup Makefile and add debug commands - Simplified .gitignore to exclude all .pid files. - Added new debug command targets in DEBUG_COMMANDS.makefile for various test scenarios. - Updated Makefile to include DEBUG_COMMANDS.makefile and added debug-related targets. - Modified BUILD.makefile to specify the binary name for installation. - Adjusted CLIENT.makefile to streamline TypeScript client generation process. * chore: Implement rust test runner - Implemented rust test runner instead of having complex makefiles and obscure code, will be easier to read and control - Cleaned up a lot of repeated constants for file names and duplicated code that was complex across our testing utils / helpers - Added a new directory for test accounts in the fixtures. - Removed unused JSON files for examples - Refactored test account structure to include additional fields for lookup tables and token accounts. - Cleaned up imports and adjusted helper functions for better organization. - Cleaned up a lot the lookup table healper, to make it more dynamic * Cleaned up test runner * Typescript test support + makefile cleanup * CI Actions updated and improvements to test runner (speed, cleanup, etc.) - Includes not hardcoded ports - Includes reuse of files to prevent too many I/O (including async io) - Exponential backoff for when we test health check to make sure its not hogging resources - Using kora binary instead of rebuilding * PR comment fixes --------- Co-authored-by: amilz <[email protected]>
📊 TypeScript Coverage ReportCoverage: 81.7% View detailed reportCoverage artifacts have been uploaded to this workflow run. |
#224) **Cache issue for github action, tested locally successfully** * feat: (PRO-255) add adversarial tests for fee payer policy violations and other scenarios - Introduced new tests for fee payer policy violations, including SOL and SPL transfer restrictions. - Added adversarial test suite to validate fee payer behavior under restrictive policies. - Updated configuration files for testing scenarios and added new test cases for various fee payer policy violations. - Refactored existing test utilities to support new test cases and improved error assertions. * PR Fixes
* feat: Some fixes post discussions with Audit & others (PRO-342, PRO-341, PRO-340, PRO-339) - Price source should not optional anywhere - Double fee counting with ATA calculation and then fee payer outflow calculation, removed ATA separated calc completely - Checked math operations everywhere to prevent overflow errors - .unwrap() removed with better graceful error handling -remove signature from response in signAndSendTransaction and signTransaction * Pr fixes * CI fixes * Fix testing with rpc not optional * Updated ts types and docs
docs/add x402 kora demo PRO-353
…ype (Free -> instant, Fixed -> just calculate price), Margin -> the full calculation (#226)
* feat: (PRO-345) Fixed inner instructions - Inner instructions weren't properly being handled - Added parsing of Parsed / PartiallyDecoded instructions coming back from the RPC - Added tests with inner instructions to make sure the inner instructions are properly used
…erloading of the server (#230)
#232) * chore: (PRO-411) (PRO-415) Clean up and sanitize DEBUG macros, loggers, etc. to prevent sensitive information from being displayed
#233) * feat: (PRO-413) Enhance fee payer outflow calculation to include SPL token transfers - Updated `calculate_fee_payer_outflow` to account for SPL token transfers, improving accuracy in fee estimation. - Added comprehensive tests for SPL token transfer scenarios to ensure correct fee calculations. - Adjusted transaction validation to utilize updated outflow calculations, ensuring consistency across transaction processing. * PR Fixes and cleaned up unwraps
…... (#234) * bugfix: Constant time hmac verification function * feat: (PRO-439) (PRO-440) Method validation for allowed methods to error out faster and reduce useless processing
* Allowed methods middleware is now separate instead of only being available when auth is enabled.
… Token2022 instructions and System instructions (#236) * feat: (PRO-414) Add rest of system instructions and spl transfer instructions for fee payer policy * feat: (PRO-414) Extend fee payer policy to include additional SPL and Token2022 instructions and System instructions - Added support for SPL token operations: revoke, set authority, mint to, initialize mint, initialize account, initialize multisig, freeze account, and thaw account. - Updated fee payer policy configuration in `kora.toml` and related files to reflect new permissions. - Enhanced validation logic to enforce these new policies during transaction processing. - Comprehensive tests added to ensure correct behavior for new fee payer policy features.
…on (#239) - Now support multi transfers for a payment (so user can transfer from 2 different token accounts to pay for the txn fee) - Refactored `has_payment_instruction` to `analyze_payment_instructions`, which now returns a tuple indicating the presence of payment instructions and the total transfer fees. - Improved fee calculation logic for SPL token transfers, including handling for Token2022 fees. - Added logging for overflow scenarios during payment accumulation. - Introduced new tests for analyzing payment instructions, including cases with multiple payments and insufficient funds. - Updated related test cases to reflect the new method and its functionality.
* bugfix: Post audit report fixes - Removed outdated dependencies such as `jup-ag` and updated versions for several packages in `Cargo.lock`. - Enhanced `ConfigValidator` to include warnings for security risks related to token extensions and authentication configurations. - Updated fee payer policies to default to more secure settings, ensuring better protection against unauthorized transfers. - Refactored several structs to implement `Default` for improved initialization. - Adjusted transaction fee response types for consistency and accuracy in handling fees. - Use of fixed point decimals for price manipulation - Constant equal for API KEY comparaison * Remove get all signers function for added safety
…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 (#241)
#242) * chore: Update dependencies and refactor token interfaces (update to v3 for sdks) - Upgraded `agave-feature-set` and `agave-reserved-account-keys` to version 3.0.8 in `Cargo.lock`. - Updated various Solana SDK dependencies to version 3.0.x for improved compatibility and features. - Refactored token-related imports to use the new `spl_token_interface` and `spl_token_2022_interface` for better modularity. - Removed outdated dependencies and adjusted related code to ensure consistency across the codebase. - Enhanced test cases to reflect changes in token interface usage and ensure proper functionality.
* chore: Audit - A09 - update CONFIGURATION.md & FEES.md to provide security guidance for handling free/fixed fees and fee payer policies - add security warnings for FREE configuration to config validator Closes PRO-504 * chore: Audit - A21 - update CONFIGURATION.md to warn about permanent delegate risk closes PRO-505 * chore: Audit - B06 - Updates FEES.md to specifiy that calculation model is only used for margin option. Renamed "Price Adjustment" to "Margin Adjustment" and improved clarity of explanation to reflect actual behavior - Nit: updated Last updated of CONFIGURATION.md Closes PRO-506 * chore: Audit - B10 - Update CONFIGURATION.md to include additional detail about the permanent limits of `useage_limit` Closes PRO-507 * chore: Audit - B11 - updates CONFIGURATION.md with increased warning for feepayer policies (note: seperate PR will update this based on new fee payer policy configuration) Closes PRO-508 * chore: revert PriceModel Free warnings
* chore: Add more warnings for configuration - Fetch mints and validate for PermanentDelegate and TransferHook extension - Added warnings for all "true" fee payer policy flags with an explanation of what the risk could be * Added "strict" option to fixed fee type, if true, will error out if the total fee is bigger than the fixed fee
…245) - Modified `TransactionUtil::new_unsigned_versioned_transaction_resolved` to return a `Result` type, ensuring proper error handling. - Updated various test cases and transaction validation logic to unwrap results, improving robustness and clarity in error management. - Introduced a new utility function `get_account_key_required` in `IxUtils` for safer account key retrieval, enhancing error reporting for missing keys.
* chore: update ADDING_SIGNERS.md update based on using the solana signers crate closes PRO-513 * chore: update docs with new Feepayer Policy Config - update CLAUDE.md - update CONFIGURATION.md - update FEES.md - update x402 guide closes PRO-501
…… (#221)
Important
Implemented
TurnkeyErrorhandling inTurnkeySignerand enhancedPrivySignererror logging, with added tests and coverage update.TurnkeyErrorenum intypes.rsfor detailed error handling inTurnkeySigner.sign()andcreate_stamp()insigner.rsto returnTurnkeyErrorvariants.PrivySignerfor API call failures insigner.rs.TurnkeySignerinitialization inconfig.rs.TurnkeyErrordisplay and conversion toKoraErrorintypes.rs.signer.rs.coverage.json.This description was created by
for 178fa5a. You can customize this summary. It will automatically update as commits are pushed.
📊 Unit Test Coverage
Unit Test Coverage: 80.8%
View Detailed Coverage Report