-
Notifications
You must be signed in to change notification settings - Fork 208
refactor: limit access to config's singleton #263
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
Conversation
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 b6d780f in 2 minutes and 3 seconds. Click for details.
- Reviewed
3325lines of code in16files - Skipped
0files when reviewing. - Skipped posting
7draft 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/validator/transaction_validator.rs:25
- Draft comment:
The field _price_source is stored and initialized (line 25 and line 51) but never used in any validation. Consider removing it if it isn’t needed. - 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.
2. crates/lib/src/validator/transaction_validator.rs:168
- Draft comment:
The validation of system and SPL instructions relies on custom macros (e.g. validate_system! and validate_spl!). Ensure these macros are well‐documented and handle all edge cases for clarity and maintainability. - 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/validator/transaction_validator.rs:80
- Draft comment:
In fetch_and_validate_token_mint, the error returned when the mint isn’t in the allowed_tokens list could include additional context (e.g. what tokens are allowed) to help debugging. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. crates/lib/src/validator/transaction_validator.rs:369
- Draft comment:
The strict pricing validation (validate_strict_pricing_with_fee) is implemented clearly. Consider adding more inline comments explaining the rationale behind the fixed price versus total fee calculation for future maintainers. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment suggests adding inline comments to explain the rationale behind a calculation. This is a request for more documentation, which is not allowed according to the rules. The comment does not provide a specific code suggestion or identify a potential issue with the code itself.
5. crates/lib/src/validator/transaction_validator.rs:101
- Draft comment:
The validate_transaction method begins by checking for empty instructions and account keys. This is a good practice; ensure that this early validation is comprehensive, especially when inner instructions might be parsed later. - 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:142
- Draft comment:
The signature validation simply checks length and emptiness. Ensure that this is sufficient and that any cryptographic signature verification is handled elsewhere. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
7. crates/lib/src/token/token.rs:786
- Draft comment:
Typographical error: The numeric literal is written as1,000,000with commas. In Rust, integer literals should use underscores (e.g.1_000_000) or no separators, as commas will result in a compilation error. Please fix this. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_IPnE1v8tzjwDcgxd
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
amilz
left a comment
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.
lgtm -- per our chat let's just make sure to have a proper release/branch plan before merging
Work done by @lgalabru
Commits of #247 but rebased on main / newer version
(From https://github.com/txtx/kora/tree/fix/limit-singletons-access)
Important
Refactor code to pass
configas a parameter to functions, improving modularity and reducing global state dependency.configas a parameter to functions instead of accessing it globally.validate_transaction,check_transaction_usage_limit, andvalidate_with_result.transaction/versioned_transaction.rs: UpdatesVersionedTransactionResolvedmethods to acceptconfig.usage_limit/usage_tracker.rs: Modifiescheck_transaction_usage_limitto requireconfig.validator/config_validator.rs: Changesvalidate_with_resultto useconfigparameter.This description was created by
for b6d780f. You can customize this summary. It will automatically update as commits are pushed.
📊 Unit Test Coverage
Unit Test Coverage: 81.0%
View Detailed Coverage Report