feat (PRO-254): Add Token 2022 integration tests#203
Conversation
|
Draft as there are more tests that need to be added |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 7abba98 in 1 minute and 59 seconds. Click for details.
- Reviewed
969lines of code in10files - Skipped
0files when reviewing. - Skipped posting
8draft 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. makefiles/RUST_TESTS.makefile:32
- Draft comment:
Good addition of the test-token target. It consistently starts/stops the validator and runs setup. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. tests/src/common/constants.rs:33
- Draft comment:
USDC_MINT_2022_KEYPAIR_PATH is defined consistently. The use of concat! with env!("CARGO_MANIFEST_DIR") is clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. tests/src/common/helpers.rs:102
- Draft comment:
Both USDCMintTestHelper and USDCMint2022TestHelper share similar logic for keypair reading. Consider abstracting the common logic to reduce duplication. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. tests/src/common/setup.rs:175
- Draft comment:
In create_usdc_mint_2022, the decimals are fetched using USDCMintTestHelper::get_test_usdc_mint_decimals(). Verify if Token2022 mints should share the same decimals or need a dedicated constant. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. tests/src/common/setup.rs:307
- Draft comment:
Token 2022 account creation instructions are combined with regular SPL account creation in one transaction. Ensure that the ordering and idempotency meet your intended behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. tests/src/common/transaction.rs:193
- Draft comment:
The with_spl_token_2022_transfer_checked builder function mirrors the SPL transfer_checked logic well. Review error handling from Token2022Program::new() to maintain consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. tests/tokens/token_2022_test.rs:15
- Draft comment:
The Token 2022 tests are comprehensive (covering legacy, v0, lookup, sign/send, and sign-if-paid scenarios). To reduce duplication, consider refactoring common setup logic in these tests. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
8. makefiles/RUST_TESTS.makefile:36
- Draft comment:
Typographical consistency: There is an inconsistency between the header label in line 33 ('TOKEN TESTS') and the integration phase description in line 36 ('Tokens Tests'). Consider using consistent terminology (either singular or plural) to avoid confusion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The inconsistency is minor and doesn't affect functionality. The meaning is clear in both cases. Looking at other test targets, there's already some variation in capitalization and pluralization (e.g., "REGULAR TESTS" vs "Regular Tests"). This suggests the project doesn't have strict naming conventions for these headers. The inconsistency could potentially cause confusion when searching through logs or documentation. Also, maintaining consistent terminology is generally good practice. While consistency is good, this is an extremely minor stylistic issue that doesn't impact functionality or readability. The comment violates the rule about not making purely informative comments that don't clearly require code changes. Delete this comment as it's too minor and doesn't highlight a significant issue that requires fixing.
Workflow ID: wflow_yHS11Ge2DLIgwAYH
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
21dc2c0 to
f9ff860
Compare
- Introduced new `test-token` target in Makefile for running Token tests. - Added `USDC_MINT_2022_KEYPAIR_PATH` constant for local testing. - Enhanced `TestAccountInfo` and `TestAccountSetup` structs to include Token 2022 fields and methods. - Implemented `create_usdc_mint_2022` method for minting Token 2022. - Added comprehensive tests for Token 2022 transfer and signing transactions. - Updated existing test utilities to support Token 2022 functionality. feat: Testing of blocked extensions on token account and mint account - Introduced `ExtensionHelpers` for creating interest-bearing mints and token accounts with memo transfer extensions. - Added constants for interest-bearing mint keypair path. - Implemented tests to validate the blocking of memo transfer and interest-bearing config extensions in transactions. - Updated transaction builder to support SPL Token 2022 transfer with specific accounts. - Enhanced test fixtures and configuration for local testing of interest-bearing mints.
f9ff860 to
d88f1ce
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to a919665 in 1 minute and 59 seconds. Click for details.
- Reviewed
1487lines of code in15files - Skipped
0files when reviewing. - Skipped posting
5draft 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/src/common/setup.rs:82
- Draft comment:
The current loop waiting for the slot to advance relies on a fixed sleep duration. For more robustness, consider using an RPC subscription or polling mechanism that waits until the desired slot is confirmed. - 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. tests/src/common/transaction.rs:312
- Draft comment:
When building signed transactions, the code iterates over signers and matches against static account keys. In scenarios with multiple signers, ensure that all required signatures are properly placed, and consider adding a helper for signing to avoid potential index mismatches. - 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. tests/src/common/extension_helpers.rs:48
- Draft comment:
Ensure that the interest-bearing mint creation correctly uses the intended extension. Verify that the calculated account space using ExtensionType::try_calculate_account_len with InterestBearingConfig meets the requirements of the mint in all testing scenarios. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify and ensure that the implementation is correct and meets requirements in all scenarios. This falls under the category of asking the author to double-check and ensure behavior, which is against the rules.
4. tests/tokens/token_2022_test.rs:444
- Draft comment:
The integration tests for Token 2022 transactions are comprehensive. Consider adding additional tests to handle error conditions (for example, simulating a failure scenario) to further improve coverage. - 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 adding error condition tests could be valuable, this comment is more of a general suggestion rather than pointing out a specific issue. The tests already verify transaction simulation success. The comment doesn't identify specific error cases that are missing or problematic. It's the kind of "nice to have" suggestion that doesn't clearly indicate what needs to change. The suggestion to add error tests could be valid - testing failure modes is important. But without specifying which error conditions are critical to test, is this comment actionable enough? While error testing is good practice, the comment is too vague to be actionable. It doesn't identify specific missing error cases that would expose real problems. Delete this comment. While the suggestion has merit, it's too general and doesn't clearly indicate what code changes are needed. It's more of an informational comment than one requiring specific action.
5. makefiles/RUST_TESTS.makefile:33
- Draft comment:
Typographical consistency: The header printed at this line is "TOKEN TESTS", but the test phase name passed to run_integration_phase on line 36 is "Tokens Tests". Consider using consistent capitalization (e.g. either "TOKEN TESTS" or "Tokens Tests") to avoid confusion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The inconsistency is real, but is it important enough to warrant a comment? This is purely cosmetic and doesn't affect functionality. Looking at the rules, we should not make purely informative comments or comments about obvious/unimportant things. The capitalization difference is very minor and doesn't impact understanding or maintainability. The inconsistency could theoretically make the codebase look less professional and might confuse developers about which naming convention to follow in the future. While consistency is good, this is too minor to warrant a PR comment. The makefile is still perfectly functional and clear despite the slight naming difference. This comment should be deleted as it's too minor and doesn't point out any real issues that need fixing.
Workflow ID: wflow_Bn3dluqV22NB71Xp
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed c670aa2 in 46 seconds. Click for details.
- Reviewed
38lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft 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/oracle/jupiter.rs:161
- Draft comment:
Good use of the SOL_MINT constant in tests instead of hardcoding. This improves consistency with production code. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. crates/lib/src/oracle/jupiter.rs:269
- Draft comment:
Using Matcher::Exact with an expected query string ensures strict query matching. Note, this test may break if query parameter order changes. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. crates/lib/src/oracle/jupiter.rs:289
- Draft comment:
Verifying that the mock was called with _m.assert() is a good practice to ensure the expected API call was made. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_RI04F8tsxML3ZNFt
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.
LGTM--only thing might be in our blocked extension case to test against additional methods but I think this is good for now.
There was a problem hiding this comment.
Would you want to add additional tests for other methods than signTransactionIfPaid?
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed f13eeac in 1 minute and 47 seconds. Click for details.
- Reviewed
88lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft 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/oracle/jupiter.rs:169
- Draft comment:
Minor style change: Removal of the extra block wrapping the GLOBAL_JUPITER_API_KEY write lock is fine, but ensure consistency across tests. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. crates/lib/src/oracle/jupiter.rs:263
- Draft comment:
Using Matcher::Any for query matching reduces test strictness. Consider using a more specific matcher to verify correct query parameters. - 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% The test is specifically checking error handling when price data is missing from the response. The exact query parameters are not critical to this test case. Adding stricter query matching would add complexity without improving the test's ability to verify the core functionality. The previous test already verifies query behavior. The comment does have a valid point about test strictness in general. Stricter tests can catch more issues. While stricter tests are generally good, in this specific case the query parameters are not relevant to the behavior being tested. Adding complexity here would reduce test readability. The comment should be deleted as it suggests adding complexity that isn't relevant to the test's purpose of verifying error handling.
3. crates/lib/src/oracle/jupiter.rs:290
- Draft comment:
Removal of the _m.assert() call means the test no longer verifies that the mock endpoint was hit. Confirm if this is intentional. - 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_iyrBM3Eqx23eKnaO
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 46ab29f in 53 seconds. Click for details.
- Reviewed
240lines of code in3files - 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/oracle/jupiter.rs:160
- Draft comment:
Removed unused import 'SOL_MINT' from the tests module. This cleanup is appropriate. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. crates/lib/src/validator/config_validator.rs:680
- Draft comment:
Removed the unused declaration of 'mock_account' in tests. This clean-up removes dead code. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. tests/tokens/token_2022_extensions_test.rs:108
- Draft comment:
The test 'test_blocked_memo_transfer_extension' asserts that the error contains a specific substring. Consider using more robust error matching (or defining error constants) to reduce brittleness if error messages change. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. tests/tokens/token_2022_extensions_test.rs:292
- Draft comment:
In the transfer fee tests, hard-coded payment amounts (e.g. 10,000 and 10,101) are used. Consider computing these expected values dynamically to avoid magic numbers and to ensure correct rounding when fee percentages change. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. tests/tokens/token_2022_extensions_test.rs:249
- Draft comment:
The ATA creation code is repeated across tests. Consider refactoring this into a helper function to reduce duplication and improve maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. tests/tokens/token_2022_extensions_test.rs:418
- Draft comment:
The overall integration tests for Token 2022 extensions are comprehensive. Ensure that error messages (e.g., blocked mint/interest-bearing errors) remain stable, as the tests rely on specific substrings. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_XB4MBqOPpKPAxSXv
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed a17f69d in 1 minute and 39 seconds. Click for details.
- Reviewed
143lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft 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/oracle/jupiter.rs:165
- Draft comment:
Consider splitting the comprehensive test into separate functions so each scenario (no API key, with API key, missing price data) is isolated. This aids clarity and prevents global state interference. - 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% The test is well-structured with clear separation between scenarios using blocks and comments. Each scenario has its own mock server and oracle instance. While splitting could provide more isolation, the current structure is clear and maintainable. The global state is properly managed within scoped blocks. The suggestion is a style preference rather than a clear improvement. The comment has merit - separate functions could make the tests more isolated. However, the current implementation already handles isolation well with scoped blocks. While splitting tests can be beneficial, the current implementation is already well-structured and clear. The suggested change would be a refactor without clear benefits. The comment should be removed as it suggests a style change that wouldn't clearly improve the code quality. The current implementation is already well-structured.
2. crates/lib/src/oracle/jupiter.rs:169
- Draft comment:
The test modifies the global mutable state (GLOBAL_JUPITER_API_KEY). Ensure that tests reset this state after execution to avoid cross-test interference when tests run concurrently. - Reason this comment was not posted:
Comment was on unchanged code.
3. crates/lib/src/oracle/jupiter.rs:262
- Draft comment:
Comparing the error by matching the exact string ("No price data from Jupiter") might be brittle. Consider matching on the error variant or using pattern matching to allow for future changes in error formatting. - 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_m2zmiq6ubBNCNE4P
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 7e8188d in 31 seconds. Click for details.
- Reviewed
7lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft 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. .github/badges/coverage.json:1
- Draft comment:
File looks good. Consider ensuring a trailing newline at EOF for consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_lUmKwdViHSQruENo
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
test-tokentarget in Makefile for running Token tests.USDC_MINT_2022_KEYPAIR_PATHconstant for local testing.TestAccountInfoandTestAccountSetupstructs to include Token 2022 fields and methods.create_usdc_mint_2022method for minting Token 2022.Important
Add integration tests and utilities for Token 2022, enhancing test coverage and support for new token features.
test-tokentarget inRUST_TESTS.makefilefor Token 2022 tests.token_2022_test.rsandtoken_2022_extensions_test.rs.USDC_MINT_2022_KEYPAIR_PATHandINTEREST_BEARING_MINT_KEYPAIR_PATHinconstants.rsfor local testing.ExtensionHelpersinextension_helpers.rsfor creating Token 2022 accounts with specific extensions.TestAccountInfoandTestAccountSetupinhelpers.rsandsetup.rsto include Token 2022 fields and methods.kora-test.tomlto include Token 2022 configurations.versioned_transaction.rsandconfig_validator.rsfor better handling of Token 2022.This description was created by
for 7e8188d. You can customize this summary. It will automatically update as commits are pushed.
📊 Test Coverage
Coverage: 86.3%
View Detailed Coverage Report