refactor: limit access to config's singleton#263
Merged
Conversation
Contributor
There was a problem hiding this comment.
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
approved these changes
Nov 24, 2025
Contributor
amilz
left a comment
There was a problem hiding this comment.
lgtm -- per our chat let's just make sure to have a proper release/branch plan before merging
amilz
pushed a commit
that referenced
this pull request
Jan 6, 2026
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.
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