feat: Add max query payment support to Query #1347
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1347 +/- ##
==========================================
+ Coverage 92.44% 92.47% +0.02%
==========================================
Files 139 139
Lines 8528 8555 +27
==========================================
+ Hits 7884 7911 +27
Misses 644 644 🚀 New features to boost your workflow:
|
|
Hi, this is WorkflowBot.
|
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
@manishdait , this is looking great!
The new default max_query_payment (1 HBAR) is correctly enforced, but it’s causing the Run Examples workflow to fail.
In examples/contract/ethereum_transaction.py, a ContractCallQuery is executed without setting max_query_payment. The reported query cost (~1.42 HBAR) exceeds the new default, so _before_execute() raises as expected.
Other examples and integration tests in this PR already follow the new pattern:
cost = query.get_cost(client)
query.set_max_query_payment(cost)
result = query.execute(client)
What do you think of updating the Ethereum transaction example to explicitly set max_query_payment in the same way?
📝 WalkthroughWalkthroughAdds per-query and client-wide maximum query payment controls. New Query.set_max_query_payment and Client.set_default_max_query_payment; queries fetch cost before execution and validate it against the per-query max or client default, raising an error if exceeded. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/contract/ethereum_transaction.py (1)
149-164: Consider simplifying the cost-fetch-and-set pattern for better UX.This is the third example file with the same cost-fetch-and-set pattern. While consistent across examples, the pattern remains redundant—fetching the cost just to set
max_query_paymentto that exact value doesn't effectively demonstrate protection against high query fees.For better user experience and clearer demonstration of the feature, consider:
Option 1: Set client default once in
setup_client():client.set_default_max_query_payment(Hbar(10))Option 2: Set a fixed max per query without fetching:
query = ( ContractCallQuery() .set_contract_id(contract_id) .set_gas(2000000) .set_function("getMessage") .set_max_query_payment(Hbar(5)) ) result = query.execute(client)If the cost-fetch pattern is intentional to guarantee execution success, consider adding a comment explaining why this approach is used rather than setting a client default.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.mdexamples/contract/contract_call_query.pyexamples/contract/contract_execute_transaction.pyexamples/contract/ethereum_transaction.pysrc/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/query/query.pytests/integration/contract_call_query_e2e_test.pytests/unit/client_test.pytests/unit/query_test.py
🧰 Additional context used
📓 Path-based instructions (3)
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: You are acting as a senior maintainer reviewing SDK examples. Your goal is to ensure examples work verbatim for users who copy-paste them.Priority 1 - Correctness:
- Verify transaction lifecycle chain (construction -> freeze_with -> sign -> execute).
- Ensure
freeze_with(client)is called BEFORE signing.- Validate that methods referenced actually exist in the
hiero_sdk_pythoncodebase.- Ensure response validation checks
receipt.statusagainstResponseCodeenums (e.g.,ResponseCode.SUCCESS).Priority 2 - Transaction Lifecycle:
- Check method chaining logic.
- Verify correct signing order (especially for multi-sig).
- Ensure explicit
.execute(client)calls.- Verify response property extraction (e.g., using
.token_id,.account_id,.serial_numbers).- Ensure error handling uses
ResponseCode(receipt.status).namefor clarity.Priority 3 - Naming & Clarity:
- Enforce role-based naming:
operator_id/_key,treasury_account_id/_key,receiver_id/_key.- Use
_idsuffix for AccountId and_keysuffix for PrivateKey variables.- Validate negative examples explicitly check for failure codes (e.g.,
TOKEN_HAS_NO_PAUSE_KEY).- Ensure logical top-to-bottom flow without ambiguity.
Priority 4 - Consistency:
- Verify standard patterns:
def main(),if __name__ == "__main__":,load_dotenv().- IMPORT RULES:
- Accept both top-level imports (e.g.,
from hiero_sdk_python import PrivateKey) and fully qualified imports (e.g.,from hiero_sdk_python.crypto.private_key import PrivateKey).- STRICTLY validate that the import path actually exists in the project structure. Compare against other files in
/examplesor your knowledge of the SDK file tree.- Flag hallucinations immediately (e.g.,
hiero_sdk_python.keysdoes not exist).- Check for
try-exceptblocks withsys.exit(1)for critical failures.Priority 5 - User Experience:
- Ensure comments explain SDK usage patterns (for users,...
Files:
examples/contract/ethereum_transaction.pyexamples/contract/contract_execute_transaction.pyexamples/contract/contract_call_query.py
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/client_test.pytests/unit/query_test.py
tests/integration/**/*
⚙️ CodeRabbit configuration file
tests/integration/**/*: You are acting as a senior maintainer reviewing integration tests for the hiero-sdk-python project. Your goal is to ensure end-to-end tests validate real network behavior safely and deterministically.CRITICAL PRINCIPLES - Safety & Diagnosability:
- Prioritize safety: No implicit or default mainnet usage.
- Secrets and credentials must be injected safely (env vars, not hardcoded).
- Test failures must be diagnosable with clear error messages.
- Tests must assert observable network behavior, not just
SUCCESS.- Failure-path tests must assert specific
ResponseCodevalues (e.g.,TOKEN_HAS_NO_PAUSE_KEY).PRIORITY 1 - End-to-End Behavior:
- Tests should be end-to-end: construct → freeze → sign → execute → verify.
- Validate resulting balances, ownership, and state changes (not just transaction success).
- Assert transaction receipts contain expected data (IDs, serial numbers, etc.).
- Verify network state after operations (e.g., account balance changed, token transferred).
PRIORITY 2 - Test Structure & Maintainability:
- One major behavior per test (clear focus).
- Tests should be readable: clear names, brief docstrings, key inline comments.
- Minimal abstraction layers - prefer clarity over DRY.
- Is the file too monolithic? Flag if tests should be split into smaller modules.
- Are helper functions good candidates for pytest fixtures or shared utilities?
PRIORITY 3 - Isolation & Cleanup:
- Local account creation over operator reuse (avoid state pollution).
- Are accounts, tokens, and allowances properly cleaned up to avoid state leakage?
- Recommend teardown strategies or fixture scoping improvements.
- Tests should not depend on execution order (avoid brittle assumptions).
PRIORITY 4 - Assertions & Coverage:
- Do tests validate only success/failure, or also assert resulting state?
- Suggest additional assertions that would strengthen correctness (balances, allowances, ownership).
- Cover happy paths AND ...
Files:
tests/integration/contract_call_query_e2e_test.py
🧬 Code graph analysis (8)
examples/contract/ethereum_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
tests/unit/client_test.py (2)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)src/hiero_sdk_python/client/client.py (4)
Client(32-287)from_env(59-103)for_testnet(106-115)set_default_max_query_payment(249-274)
src/hiero_sdk_python/client/client.py (1)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)
examples/contract/contract_execute_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
src/hiero_sdk_python/query/query.py (1)
src/hiero_sdk_python/hbar.py (2)
Hbar(18-213)to_hbars(67-71)
tests/unit/query_test.py (3)
src/hiero_sdk_python/query/query.py (4)
set_max_query_payment(96-124)_is_payment_required(414-421)get_cost(281-321)_before_execute(126-157)src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)src/hiero_sdk_python/client/client.py (1)
set_default_max_query_payment(249-274)
tests/integration/contract_call_query_e2e_test.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
execute(190-216)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
examples/contract/contract_call_query.py (1)
src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
🪛 Ruff (0.14.10)
tests/unit/client_test.py
192-192: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
src/hiero_sdk_python/client/client.py
7-7: typing.List is deprecated, use list instead
(UP035)
249-249: Use X | Y for type annotations
Convert to X | Y
(UP007)
263-265: Avoid specifying long messages outside the exception class
(TRY003)
269-269: Avoid specifying long messages outside the exception class
(TRY003)
src/hiero_sdk_python/query/query.py
96-96: Use X | Y for type annotations
Convert to X | Y
(UP007)
113-115: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
154-157: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/query_test.py
251-251: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
254-254: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (12)
CHANGELOG.md (1)
81-81: LGTM! Clear and accurate changelog entry.The changelog entry properly documents both the per-query and client-wide maximum payment configuration methods introduced in this PR.
tests/unit/client_test.py (1)
191-232: Excellent test coverage for set_default_max_query_payment!The test suite comprehensively covers:
- ✅ Valid input types (int, float, Decimal, Hbar) with correct conversion
- ✅ Default value verification (1 Hbar)
- ✅ Negative values raising ValueError with descriptive messages
- ✅ Invalid types raising TypeError with descriptive messages
The tests follow unit testing best practices and will effectively protect against breaking changes.
tests/integration/contract_call_query_e2e_test.py (1)
132-179: LGTM: test validates get_cost() with manual payment setting.This test appropriately uses
set_query_payment()to manually override the payment rather thanset_max_query_payment(), which is the correct pattern for validating cost calculation with explicit payment amounts.src/hiero_sdk_python/query/query.py (2)
5-5: LGTM: Import additions support new feature.The
Decimalimport is necessary for the type validation inset_max_query_payment(), and the import reorganization is clean.Also applies to: 22-22
96-124: LGTM: Robust input validation and conversion.The method correctly validates input types, handles negative values, and converts numeric types to
Hbar. The fluent interface (returningself) is properly maintained.Static analysis hints about long error messages (TRY003) and Union syntax (UP007) are stylistic preferences and don't impact functionality.
src/hiero_sdk_python/client/client.py (3)
5-5: LGTM: Well-defined constant with appropriate default.The
DEFAULT_MAX_QUERY_PAYMENT = Hbar(1)provides a sensible baseline for query cost protection. Imports support the type validation logic.Also applies to: 11-11, 23-24
52-52: LGTM: Proper initialization from module constant.Instance variable allows per-client customization while maintaining a sensible default.
249-274: LGTM: Validation logic mirrors Query.set_max_query_payment.The implementation correctly validates input types and values, matching the pattern in
Query.set_max_query_payment(). The code duplication is acceptable given it only appears in two locations and centralizing it might reduce clarity.Static analysis hints (TRY003, UP007) are stylistic and don't impact functionality.
tests/unit/query_test.py (4)
1-2: LGTM: Necessary imports for comprehensive test coverage.
Decimalenables testing ofDecimalinput types, andreis needed for precise error message matching with special characters (ℏ).
60-60: LGTM: Test updated to accommodate new validation.Setting
max_query_paymentto 3 Hbar when the mocked cost is 2 Hbar ensures the payment validation passes, which is the expected behavior for this test.
253-289: Excellent coverage of input validation scenarios.The parameterized tests comprehensively validate:
- Type conversions (int, float, Decimal, Hbar)
- Zero and positive values
- Negative value rejection
- Invalid type rejection
Error message matching correctly uses type names and values.
Minor style note: Static analysis suggests using a tuple for the first argument of
@pytest.mark.parametrizeon line 254 (e.g.,('valid_amount', 'expected')instead of'valid_amount,expected'), but this is cosmetic and both forms work.
291-376: Comprehensive test coverage for query/client max payment interactions.These tests thoroughly validate:
- Query-specific
max_query_paymentoverriding client default- Fallback to
client.default_max_query_paymentwhen not set- Both success scenarios (cost ≤ max) and failure scenarios (cost > max)
- Correct error message formatting
The mocked cost values ensure deterministic test behavior, and the test names clearly describe the scenarios being validated.
| query = ( | ||
| ContractCallQuery() | ||
| .set_contract_id(contract_id) | ||
| .set_gas(2000000) | ||
| .set_function("getMessage") | ||
| .execute(client) | ||
| ) | ||
|
|
||
| cost = query.get_cost(client) | ||
| query.set_max_query_payment(cost) | ||
|
|
||
| result = query.execute(client) | ||
|
|
||
| # The contract returns bytes32, which we decode to string | ||
| # This removes any padding and converts to readable text | ||
| return result.get_bytes32(0).decode("utf-8") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying the cost-fetch-and-set pattern for better UX.
The current pattern fetches the cost and immediately sets max_query_payment to that exact value. While functionally correct, this approach is somewhat redundant—setting the max to exactly the fetched cost doesn't demonstrate protection against unexpectedly high fees.
For better user experience, consider one of these alternatives:
Option 1: Set a reasonable client-wide default once at setup:
def setup_client():
"""Initialize and set up the client with operator account"""
network = Network(network_name)
client = Client(network)
operator_id = AccountId.from_string(os.getenv("OPERATOR_ID", ""))
operator_key = PrivateKey.from_string(os.getenv("OPERATOR_KEY", ""))
client.set_operator(operator_id, operator_key)
client.set_default_max_query_payment(Hbar(10)) # Set once for all queries
return clientThen queries can execute directly without the cost-fetch step:
result = (
ContractCallQuery()
.set_contract_id(contract_id)
.set_gas(2000000)
.set_function("getMessage")
.execute(client)
)Option 2: Set a fixed reasonable max per query without fetching:
result = (
ContractCallQuery()
.set_contract_id(contract_id)
.set_gas(2000000)
.set_function("getMessage")
.set_max_query_payment(Hbar(5))
.execute(client)
)Both approaches are simpler for users to copy and demonstrate the protection feature more clearly.
| if self.payment_amount > max_payment: | ||
| raise ValueError( | ||
| f"Query cost ℏ{self.payment_amount.to_hbars()} HBAR " | ||
| f"exceeds max set query payment: ℏ{max_payment.to_hbars()} HBAR" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify error message formatting.
The error message format includes redundant currency designation: "Query cost ℏ{value} HBAR" uses both the symbol (ℏ) and the word (HBAR).
Consider using Hbar's built-in string representation for consistency:
🔎 Proposed fix
if self.payment_amount > max_payment:
raise ValueError(
- f"Query cost ℏ{self.payment_amount.to_hbars()} HBAR "
- f"exceeds max set query payment: ℏ{max_payment.to_hbars()} HBAR"
+ f"Query cost {self.payment_amount} "
+ f"exceeds max set query payment: {max_payment}"
)This uses Hbar.__str__() which formats as "X.XXXXXXXX ℏ" per the Hbar class implementation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.payment_amount > max_payment: | |
| raise ValueError( | |
| f"Query cost ℏ{self.payment_amount.to_hbars()} HBAR " | |
| f"exceeds max set query payment: ℏ{max_payment.to_hbars()} HBAR" | |
| ) | |
| if self.payment_amount > max_payment: | |
| raise ValueError( | |
| f"Query cost {self.payment_amount} " | |
| f"exceeds max set query payment: {max_payment}" | |
| ) |
🧰 Tools
🪛 Ruff (0.14.10)
154-157: Avoid specifying long messages outside the exception class
(TRY003)
manishdait
force-pushed
the
feat/max_query_fee
branch
from
44b253e to
c773357
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.mdexamples/contract/contract_call_query.pyexamples/contract/contract_execute_transaction.pyexamples/contract/ethereum_transaction.pysrc/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/query/query.pytests/integration/contract_call_query_e2e_test.pytests/integration/query_e2e_test.pytests/unit/client_test.pytests/unit/query_test.py
🧰 Additional context used
📓 Path-based instructions (3)
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: You are acting as a senior maintainer reviewing SDK examples. Your goal is to ensure examples work verbatim for users who copy-paste them.Priority 1 - Correctness:
- Verify transaction lifecycle chain (construction -> freeze_with -> sign -> execute).
- Ensure
freeze_with(client)is called BEFORE signing.- Validate that methods referenced actually exist in the
hiero_sdk_pythoncodebase.- Ensure response validation checks
receipt.statusagainstResponseCodeenums (e.g.,ResponseCode.SUCCESS).Priority 2 - Transaction Lifecycle:
- Check method chaining logic.
- Verify correct signing order (especially for multi-sig).
- Ensure explicit
.execute(client)calls.- Verify response property extraction (e.g., using
.token_id,.account_id,.serial_numbers).- Ensure error handling uses
ResponseCode(receipt.status).namefor clarity.Priority 3 - Naming & Clarity:
- Enforce role-based naming:
operator_id/_key,treasury_account_id/_key,receiver_id/_key.- Use
_idsuffix for AccountId and_keysuffix for PrivateKey variables.- Validate negative examples explicitly check for failure codes (e.g.,
TOKEN_HAS_NO_PAUSE_KEY).- Ensure logical top-to-bottom flow without ambiguity.
Priority 4 - Consistency:
- Verify standard patterns:
def main(),if __name__ == "__main__":,load_dotenv().- IMPORT RULES:
- Accept both top-level imports (e.g.,
from hiero_sdk_python import PrivateKey) and fully qualified imports (e.g.,from hiero_sdk_python.crypto.private_key import PrivateKey).- STRICTLY validate that the import path actually exists in the project structure. Compare against other files in
/examplesor your knowledge of the SDK file tree.- Flag hallucinations immediately (e.g.,
hiero_sdk_python.keysdoes not exist).- Check for
try-exceptblocks withsys.exit(1)for critical failures.Priority 5 - User Experience:
- Ensure comments explain SDK usage patterns (for users,...
Files:
examples/contract/contract_execute_transaction.pyexamples/contract/ethereum_transaction.pyexamples/contract/contract_call_query.py
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/client_test.pytests/unit/query_test.py
tests/integration/**/*
⚙️ CodeRabbit configuration file
tests/integration/**/*: You are acting as a senior maintainer reviewing integration tests for the hiero-sdk-python project. Your goal is to ensure end-to-end tests validate real network behavior safely and deterministically.CRITICAL PRINCIPLES - Safety & Diagnosability:
- Prioritize safety: No implicit or default mainnet usage.
- Secrets and credentials must be injected safely (env vars, not hardcoded).
- Test failures must be diagnosable with clear error messages.
- Tests must assert observable network behavior, not just
SUCCESS.- Failure-path tests must assert specific
ResponseCodevalues (e.g.,TOKEN_HAS_NO_PAUSE_KEY).PRIORITY 1 - End-to-End Behavior:
- Tests should be end-to-end: construct → freeze → sign → execute → verify.
- Validate resulting balances, ownership, and state changes (not just transaction success).
- Assert transaction receipts contain expected data (IDs, serial numbers, etc.).
- Verify network state after operations (e.g., account balance changed, token transferred).
PRIORITY 2 - Test Structure & Maintainability:
- One major behavior per test (clear focus).
- Tests should be readable: clear names, brief docstrings, key inline comments.
- Minimal abstraction layers - prefer clarity over DRY.
- Is the file too monolithic? Flag if tests should be split into smaller modules.
- Are helper functions good candidates for pytest fixtures or shared utilities?
PRIORITY 3 - Isolation & Cleanup:
- Local account creation over operator reuse (avoid state pollution).
- Are accounts, tokens, and allowances properly cleaned up to avoid state leakage?
- Recommend teardown strategies or fixture scoping improvements.
- Tests should not depend on execution order (avoid brittle assumptions).
PRIORITY 4 - Assertions & Coverage:
- Do tests validate only success/failure, or also assert resulting state?
- Suggest additional assertions that would strengthen correctness (balances, allowances, ownership).
- Cover happy paths AND ...
Files:
tests/integration/contract_call_query_e2e_test.pytests/integration/query_e2e_test.py
🧬 Code graph analysis (8)
examples/contract/contract_execute_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
examples/contract/ethereum_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
src/hiero_sdk_python/client/client.py (1)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)
tests/unit/client_test.py (2)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)src/hiero_sdk_python/client/client.py (2)
Client(32-287)set_default_max_query_payment(249-274)
tests/unit/query_test.py (3)
src/hiero_sdk_python/query/query.py (4)
set_max_query_payment(96-124)_is_payment_required(414-421)get_cost(281-321)_before_execute(126-157)src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)src/hiero_sdk_python/client/client.py (1)
set_default_max_query_payment(249-274)
tests/integration/contract_call_query_e2e_test.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (5)
ContractCallQuery(27-233)set_contract_id(59-67)set_gas(69-77)set_function(107-124)execute(190-216)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
tests/integration/query_e2e_test.py (2)
src/hiero_sdk_python/query/account_info_query.py (1)
AccountInfoQuery(11-131)src/hiero_sdk_python/query/query.py (1)
set_max_query_payment(96-124)
examples/contract/contract_call_query.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
🪛 Ruff (0.14.10)
src/hiero_sdk_python/client/client.py
7-7: typing.List is deprecated, use list instead
(UP035)
249-249: Use X | Y for type annotations
Convert to X | Y
(UP007)
263-265: Avoid specifying long messages outside the exception class
(TRY003)
269-269: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/client_test.py
192-192: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
src/hiero_sdk_python/query/query.py
96-96: Use X | Y for type annotations
Convert to X | Y
(UP007)
113-115: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
154-157: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/query_test.py
251-251: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
254-254: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
tests/integration/query_e2e_test.py
255-255: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
259-259: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.12)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (12)
src/hiero_sdk_python/client/client.py (2)
23-24: LGTM! Reasonable default for maximum query payment.The
DEFAULT_MAX_QUERY_PAYMENT = Hbar(1)constant provides a sensible client-wide default that protects users from unexpectedly high query fees while allowing sufficient headroom for typical queries.
249-274: LGTM! Robust implementation with proper validation.The
set_default_max_query_payment()method correctly:
- Validates input types and rejects invalid types with clear error messages
- Ensures non-negative values for numeric inputs
- Converts numeric types to
Hbarobjects consistently- Returns
selffor method chainingThe static analysis hints about Union syntax and exception message style are minor preferences for Python 3.10+ and do not affect functionality.
examples/contract/contract_execute_transaction.py (1)
110-124: [Duplicate] Reconsider the cost-fetch-and-set pattern for better user guidance.As noted in previous review feedback, the current pattern (fetch cost → set max to exact cost → execute) is redundant and doesn't effectively demonstrate the protection feature. Setting
max_query_paymentto exactly the fetched cost will always pass, which defeats the purpose of showing users how to protect against unexpectedly high fees.Consider one of these simpler, more instructive alternatives:
Option 1 (Recommended): Set a client-wide default once in
setup_client():def setup_client(): """Initialize and set up the client with operator account""" network = Network(network_name) print(f"Connecting to Hedera {network_name} network!") client = Client(network) operator_id = AccountId.from_string(os.getenv("OPERATOR_ID", "")) operator_key = PrivateKey.from_string(os.getenv("OPERATOR_KEY", "")) client.set_operator(operator_id, operator_key) client.set_default_max_query_payment(Hbar(10)) # Set once for all queries print(f"Client set up with operator id {client.operator_account_id}") return clientThen queries execute directly without the cost-fetch step:
def get_contract_message(client, contract_id): """Get the message from the contract""" result = ( ContractCallQuery() .set_contract_id(contract_id) .set_gas(2000000) .set_function("getMessage") .execute(client) ) return result.get_bytes32(0).decode("utf-8")Option 2: Set a fixed reasonable max per query:
def get_contract_message(client, contract_id): """Get the message from the contract""" result = ( ContractCallQuery() .set_contract_id(contract_id) .set_gas(2000000) .set_function("getMessage") .set_max_query_payment(Hbar(5)) .execute(client) ) return result.get_bytes32(0).decode("utf-8")Both approaches are simpler for users to copy-paste and better demonstrate the protection feature.
Based on past review comments and examples coding guidelines.
Likely an incorrect or invalid review comment.
examples/contract/ethereum_transaction.py (1)
146-164: LGTM! Correctly demonstrates the two-step cost-aware query pattern.The updated
get_contract_messagefunction properly demonstrates the new workflow:
- Construct the query
- Fetch the cost with
get_cost(client)- Set the max payment with
set_max_query_payment(cost)- Execute the query
This provides a clear example for SDK users on how to use the new max query payment feature.
tests/integration/contract_call_query_e2e_test.py (1)
116-127: LGTM! Two-step pattern correctly applied.The test properly demonstrates fetching the cost before setting the max payment and executing.
src/hiero_sdk_python/query/query.py (2)
143-157: Max payment check only applies to auto-fetched costs.The max payment validation (lines 145-157) only triggers when
self.payment_amount is Noneand is set viaget_cost(). If a user manually setspayment_amountviaset_query_payment(), the max check is bypassed entirely.Is this intentional? If a user does:
query.set_max_query_payment(Hbar(1)) query.set_query_payment(Hbar(5)) # Exceeds max, but no validation query.execute(client)The query will execute without the max payment protection. Consider whether the max check should also validate manually-set payments, or document this behavior explicitly.
96-124: LGTM! Setter implementation is correct.The
set_max_query_paymentmethod correctly:
- Validates input types (int, float, Decimal, Hbar)
- Rejects negative values with a clear error message
- Converts non-Hbar inputs to Hbar
- Returns
selffor method chainingtests/unit/query_test.py (5)
328-335: Duplicate assignment removed - LGTM.The previously flagged duplicate assignment (
query_requires_payment.get_cost = mock_get_cost) appears to have been resolved. The test correctly validates that execution fails when cost exceeds the query-specific max.
366-373: Duplicate assignment removed - LGTM.The previously flagged duplicate assignment has been resolved. The test correctly validates that execution fails when cost exceeds the client's default max when no query override exists.
291-310: LGTM! Comprehensive test for query max overriding client default.The test correctly validates:
- Client has default max of 1 Hbar
- Query overrides with 2 Hbar
- Cost of 2 Hbar succeeds because it's within query's max
- Payment amount is correctly set
338-357: LGTM! Tests client default max fallback behavior.The test correctly validates that when no query-level max is set, the client's default is used, and execution succeeds when cost is within that limit.
251-251: Avoid equality comparison toTrue.Use the truthiness check directly instead of comparing to
True.🔎 Proposed fix
- assert query_requires_payment._is_payment_required() == True + assert query_requires_payment._is_payment_required()Likely an incorrect or invalid review comment.
| query = ( | ||
| ContractCallQuery() | ||
| .set_contract_id(contract_id) | ||
| .set_gas(10000000) | ||
| .set_function("getMessage") | ||
| .execute(env.client) | ||
| ) | ||
|
|
||
| cost = query.get_cost(env.client) | ||
| query.set_max_query_payment(cost) | ||
|
|
||
| result = query.execute(env.client) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test for max_query_payment enforcement when cost exceeds limit.
The current changes demonstrate the two-step cost-aware pattern but don't validate the core protection mechanism. Setting max_query_payment to exactly the fetched cost will never trigger the "exceeds max" error path.
Consider adding a dedicated test that validates the protection behavior (per issue #1346 objective: "Fail early with a descriptive error if the cost exceeds the configured maximum"):
@pytest.mark.integration
def test_integration_contract_call_query_exceeds_max_payment(env):
"""Test that ContractCallQuery fails when cost exceeds max_query_payment."""
# Deploy contract (reuse setup from existing tests)
receipt = (
FileCreateTransaction()
.set_keys(env.operator_key.public_key())
.set_contents(SIMPLE_CONTRACT_BYTECODE)
.execute(env.client)
)
file_id = receipt.file_id
receipt = (
ContractCreateTransaction()
.set_admin_key(env.operator_key.public_key())
.set_gas(CONTRACT_DEPLOY_GAS)
.set_bytecode_file_id(file_id)
.execute(env.client)
)
contract_id = receipt.contract_id
# Set max payment intentionally below actual cost
query = (
ContractCallQuery()
.set_contract_id(contract_id)
.set_gas(10000000)
.set_function("greet")
.set_max_query_payment(Hbar.from_tinybars(1)) # Intentionally too low
)
with pytest.raises(ValueError, match="exceeds max set query payment"):
query.execute(env.client)| def test_set_default_max_query_payment_valid_param(valid_amount, expected): | ||
| """Test that set_default_max_query_payment correctly converts various input types to Hbar.""" | ||
| client = Client.for_testnet() | ||
| # by default is 1 hbar before setting it | ||
| assert client.default_max_query_payment == Hbar(1) | ||
| client.set_default_max_query_payment(valid_amount) | ||
| assert client.default_max_query_payment == expected |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add test for fluent interface (method chaining).
Per coding guidelines, fluent setters should be tested to return self. The test verifies value conversion but doesn't assert that set_default_max_query_payment returns the client instance for method chaining.
🔎 Proposed fix
def test_set_default_max_query_payment_valid_param(valid_amount, expected):
"""Test that set_default_max_query_payment correctly converts various input types to Hbar."""
client = Client.for_testnet()
# by default is 1 hbar before setting it
assert client.default_max_query_payment == Hbar(1)
- client.set_default_max_query_payment(valid_amount)
+ result = client.set_default_max_query_payment(valid_amount)
+ assert result is client, "set_default_max_query_payment should return self for method chaining"
assert client.default_max_query_payment == expected
+ client.close()As per coding guidelines: "Assert fluent setters return self".
| def test_set_default_max_query_payment_negative_value(negative_amount): | ||
| """Test set_default_max_query_payment for negative amount values.""" | ||
| client = Client.for_testnet() | ||
|
|
||
| with pytest.raises(ValueError, match=f"max_query_payment must be non-negative, got {negative_amount}"): | ||
| client.set_default_max_query_payment(negative_amount) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add resource cleanup in negative value tests.
The test creates a client but doesn't close it. Consider adding cleanup for consistency with other tests in this file.
🔎 Proposed fix
def test_set_default_max_query_payment_negative_value(negative_amount):
"""Test set_default_max_query_payment for negative amount values."""
client = Client.for_testnet()
with pytest.raises(ValueError, match=f"max_query_payment must be non-negative, got {negative_amount}"):
client.set_default_max_query_payment(negative_amount)
+
+ client.close()| def test_set_default_max_query_payment_invalid_param(invalid_amount): | ||
| """Test that set_default_max_query_payment raise error for invalid param.""" | ||
| client = Client.for_testnet() | ||
|
|
||
| with pytest.raises(TypeError, match=( | ||
| f"max_query_payment must be int, float, Decimal, or Hbar, " | ||
| f"got {type(invalid_amount).__name__}" | ||
| )): | ||
| client.set_default_max_query_payment(invalid_amount) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add resource cleanup in invalid param tests.
Same cleanup consideration as the negative value tests.
🔎 Proposed fix
def test_set_default_max_query_payment_invalid_param(invalid_amount):
"""Test that set_default_max_query_payment raise error for invalid param."""
client = Client.for_testnet()
with pytest.raises(TypeError, match=(
f"max_query_payment must be int, float, Decimal, or Hbar, "
f"got {type(invalid_amount).__name__}"
)):
client.set_default_max_query_payment(invalid_amount)
+
+ client.close()| @pytest.mark.parametrize( | ||
| 'valid_amount,expected', | ||
| [ | ||
| (1, Hbar(1)), | ||
| (0.1, Hbar(0.1)), | ||
| (Decimal('0.1'), Hbar(Decimal('0.1'))), | ||
| (Hbar(1), Hbar(1)), | ||
| (Hbar(0), Hbar(0)) | ||
| ] | ||
| ) | ||
| def test_set_max_query_payment_valid_param(query, valid_amount, expected): | ||
| """Test that set_max_query_payment correctly converts various input types to Hbar.""" | ||
| # by default is none before setting it | ||
| assert query.max_query_payment is None | ||
| query.set_max_query_payment(valid_amount) | ||
| assert query.max_query_payment == expected |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add test for fluent interface (method chaining).
Per coding guidelines, fluent setters should be tested to return self. The test verifies value conversion but doesn't assert that set_max_query_payment returns the query instance.
🔎 Proposed fix
def test_set_max_query_payment_valid_param(query, valid_amount, expected):
"""Test that set_max_query_payment correctly converts various input types to Hbar."""
# by default is none before setting it
assert query.max_query_payment is None
- query.set_max_query_payment(valid_amount)
+ result = query.set_max_query_payment(valid_amount)
+ assert result is query, "set_max_query_payment should return self for method chaining"
assert query.max_query_payment == expectedAs per coding guidelines: "Assert fluent setters return self".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.parametrize( | |
| 'valid_amount,expected', | |
| [ | |
| (1, Hbar(1)), | |
| (0.1, Hbar(0.1)), | |
| (Decimal('0.1'), Hbar(Decimal('0.1'))), | |
| (Hbar(1), Hbar(1)), | |
| (Hbar(0), Hbar(0)) | |
| ] | |
| ) | |
| def test_set_max_query_payment_valid_param(query, valid_amount, expected): | |
| """Test that set_max_query_payment correctly converts various input types to Hbar.""" | |
| # by default is none before setting it | |
| assert query.max_query_payment is None | |
| query.set_max_query_payment(valid_amount) | |
| assert query.max_query_payment == expected | |
| @pytest.mark.parametrize( | |
| 'valid_amount,expected', | |
| [ | |
| (1, Hbar(1)), | |
| (0.1, Hbar(0.1)), | |
| (Decimal('0.1'), Hbar(Decimal('0.1'))), | |
| (Hbar(1), Hbar(1)), | |
| (Hbar(0), Hbar(0)) | |
| ] | |
| ) | |
| def test_set_max_query_payment_valid_param(query, valid_amount, expected): | |
| """Test that set_max_query_payment correctly converts various input types to Hbar.""" | |
| # by default is none before setting it | |
| assert query.max_query_payment is None | |
| result = query.set_max_query_payment(valid_amount) | |
| assert result is query, "set_max_query_payment should return self for method chaining" | |
| assert query.max_query_payment == expected |
🧰 Tools
🪛 Ruff (0.14.10)
254-254: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
manishdait
force-pushed
the
feat/max_query_fee
branch
from
c773357 to
8ee657c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/integration/query_e2e_test.pytests/unit/query_test.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/integration/**/*
⚙️ CodeRabbit configuration file
tests/integration/**/*: You are acting as a senior maintainer reviewing integration tests for the hiero-sdk-python project. Your goal is to ensure end-to-end tests validate real network behavior safely and deterministically.CRITICAL PRINCIPLES - Safety & Diagnosability:
- Prioritize safety: No implicit or default mainnet usage.
- Secrets and credentials must be injected safely (env vars, not hardcoded).
- Test failures must be diagnosable with clear error messages.
- Tests must assert observable network behavior, not just
SUCCESS.- Failure-path tests must assert specific
ResponseCodevalues (e.g.,TOKEN_HAS_NO_PAUSE_KEY).PRIORITY 1 - End-to-End Behavior:
- Tests should be end-to-end: construct → freeze → sign → execute → verify.
- Validate resulting balances, ownership, and state changes (not just transaction success).
- Assert transaction receipts contain expected data (IDs, serial numbers, etc.).
- Verify network state after operations (e.g., account balance changed, token transferred).
PRIORITY 2 - Test Structure & Maintainability:
- One major behavior per test (clear focus).
- Tests should be readable: clear names, brief docstrings, key inline comments.
- Minimal abstraction layers - prefer clarity over DRY.
- Is the file too monolithic? Flag if tests should be split into smaller modules.
- Are helper functions good candidates for pytest fixtures or shared utilities?
PRIORITY 3 - Isolation & Cleanup:
- Local account creation over operator reuse (avoid state pollution).
- Are accounts, tokens, and allowances properly cleaned up to avoid state leakage?
- Recommend teardown strategies or fixture scoping improvements.
- Tests should not depend on execution order (avoid brittle assumptions).
PRIORITY 4 - Assertions & Coverage:
- Do tests validate only success/failure, or also assert resulting state?
- Suggest additional assertions that would strengthen correctness (balances, allowances, ownership).
- Cover happy paths AND ...
Files:
tests/integration/query_e2e_test.py
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/query_test.py
🧬 Code graph analysis (2)
tests/integration/query_e2e_test.py (3)
src/hiero_sdk_python/query/account_info_query.py (1)
AccountInfoQuery(11-131)tests/integration/utils.py (2)
IntegrationTestEnv(34-96)env(23-27)src/hiero_sdk_python/query/query.py (1)
set_max_query_payment(96-124)
tests/unit/query_test.py (3)
src/hiero_sdk_python/query/query.py (4)
set_max_query_payment(96-124)_is_payment_required(414-421)get_cost(281-321)_before_execute(126-157)src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)src/hiero_sdk_python/client/client.py (1)
set_default_max_query_payment(249-274)
🪛 Ruff (0.14.10)
tests/integration/query_e2e_test.py
243-243: Redefinition of unused env from line 12
(F811)
255-255: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
259-259: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
tests/unit/query_test.py
251-251: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
254-254: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.10)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (1)
tests/unit/query_test.py (1)
291-374: Good test coverage for max_query_payment behavior.The tests comprehensively cover:
- Query overriding client default max payment (success case)
- Query override with cost exceeding query-specific max (error case)
- Query using client default max payment (success case)
- Query using client default with cost exceeding default (error case)
The use of
re.escape()for matching error messages with special characters (ℏ symbol) is appropriate.
| @pytest.mark.integration | ||
| def test_integration_query_exceeds_max_payment(env): | ||
| """Test that Query fails when cost exceeds max_query_payment.""" | ||
| receipt = env.create_account(1) | ||
| account_id = receipt.id | ||
|
|
||
| # Set max payment below actual cost | ||
| query = ( | ||
| AccountInfoQuery() | ||
| .set_account_id(account_id) | ||
| .set_max_query_payment(Hbar.from_tinybars(1)) # Intentionally too low to fail | ||
| ) | ||
|
|
||
| with pytest.raises(ValueError) as e: | ||
| query.execute(env.client) | ||
|
|
||
| msg = str(e.value) | ||
| assert "Query cost" in msg and "exceeds max set query payment:" in msg | ||
|
|
There was a problem hiding this comment.
Fix test implementation issues: missing match parameter and compound assertion.
The test logic is correct (using env fixture and receipt.id are appropriate based on utils.py), but there are still issues to address:
-
Missing
matchparameter (Line 255): Specify a match pattern to make the test more precise. -
Compound assertion (Line 259): Breaking this into separate assertions improves test failure diagnosis.
🔎 Proposed fix
- with pytest.raises(ValueError) as e:
+ with pytest.raises(ValueError, match=r"Query cost.*exceeds max set query payment") as e:
query.execute(env.client)
msg = str(e.value)
- assert "Query cost" in msg and "exceeds max set query payment:" in msg
+ assert "Query cost" in msg
+ assert "exceeds max set query payment:" in msg🧰 Tools
🪛 Ruff (0.14.10)
243-243: Redefinition of unused env from line 12
(F811)
255-255: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
259-259: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
tests/unit/query_test.py
Outdated
| query_requires_payment.set_max_query_payment(2) | ||
| assert query_requires_payment.max_query_payment == Hbar(2) | ||
|
|
||
| # mock the get_cost to resturn 2 hbar as required paymnet |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fix typos in comments.
Minor typos in docstring comments.
🔎 Proposed fix
- # mock the get_cost to resturn 2 hbar as required paymnet
+ # mock the get_cost to return 2 hbar as required paymentApply at both lines 302 and 349.
Also applies to: 349-349
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
|
@manishdait This is looking great 👍 Please be sure to address merge conflicts. Thank you!! |
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
manishdait
force-pushed
the
feat/max_query_fee
branch
from
8ee657c to
7b9b950
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.mdexamples/contract/contract_call_query.pyexamples/contract/contract_execute_transaction.pyexamples/contract/ethereum_transaction.pysrc/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/query/query.pytests/integration/contract_call_query_e2e_test.pytests/integration/query_e2e_test.pytests/unit/client_test.pytests/unit/query_test.py
🧰 Additional context used
📓 Path-based instructions (3)
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: You are acting as a senior maintainer reviewing SDK examples. Your goal is to ensure examples work verbatim for users who copy-paste them.Priority 1 - Correctness:
- Verify transaction lifecycle chain (construction -> freeze_with -> sign -> execute).
- Ensure
freeze_with(client)is called BEFORE signing.- Validate that methods referenced actually exist in the
hiero_sdk_pythoncodebase.- Ensure response validation checks
receipt.statusagainstResponseCodeenums (e.g.,ResponseCode.SUCCESS).Priority 2 - Transaction Lifecycle:
- Check method chaining logic.
- Verify correct signing order (especially for multi-sig).
- Ensure explicit
.execute(client)calls.- Verify response property extraction (e.g., using
.token_id,.account_id,.serial_numbers).- Ensure error handling uses
ResponseCode(receipt.status).namefor clarity.Priority 3 - Naming & Clarity:
- Enforce role-based naming:
operator_id/_key,treasury_account_id/_key,receiver_id/_key.- Use
_idsuffix for AccountId and_keysuffix for PrivateKey variables.- Validate negative examples explicitly check for failure codes (e.g.,
TOKEN_HAS_NO_PAUSE_KEY).- Ensure logical top-to-bottom flow without ambiguity.
Priority 4 - Consistency:
- Verify standard patterns:
def main(),if __name__ == "__main__":,load_dotenv().- IMPORT RULES:
- Accept both top-level imports (e.g.,
from hiero_sdk_python import PrivateKey) and fully qualified imports (e.g.,from hiero_sdk_python.crypto.private_key import PrivateKey).- STRICTLY validate that the import path actually exists in the project structure. Compare against other files in
/examplesor your knowledge of the SDK file tree.- Flag hallucinations immediately (e.g.,
hiero_sdk_python.keysdoes not exist).- Check for
try-exceptblocks withsys.exit(1)for critical failures.Priority 5 - User Experience:
- Ensure comments explain SDK usage patterns (for users,...
Files:
examples/contract/ethereum_transaction.pyexamples/contract/contract_execute_transaction.pyexamples/contract/contract_call_query.py
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/client_test.pytests/unit/query_test.py
tests/integration/**/*
⚙️ CodeRabbit configuration file
tests/integration/**/*: You are acting as a senior maintainer reviewing integration tests for the hiero-sdk-python project. Your goal is to ensure end-to-end tests validate real network behavior safely and deterministically.CRITICAL PRINCIPLES - Safety & Diagnosability:
- Prioritize safety: No implicit or default mainnet usage.
- Secrets and credentials must be injected safely (env vars, not hardcoded).
- Test failures must be diagnosable with clear error messages.
- Tests must assert observable network behavior, not just
SUCCESS.- Failure-path tests must assert specific
ResponseCodevalues (e.g.,TOKEN_HAS_NO_PAUSE_KEY).PRIORITY 1 - End-to-End Behavior:
- Tests should be end-to-end: construct → freeze → sign → execute → verify.
- Validate resulting balances, ownership, and state changes (not just transaction success).
- Assert transaction receipts contain expected data (IDs, serial numbers, etc.).
- Verify network state after operations (e.g., account balance changed, token transferred).
PRIORITY 2 - Test Structure & Maintainability:
- One major behavior per test (clear focus).
- Tests should be readable: clear names, brief docstrings, key inline comments.
- Minimal abstraction layers - prefer clarity over DRY.
- Is the file too monolithic? Flag if tests should be split into smaller modules.
- Are helper functions good candidates for pytest fixtures or shared utilities?
PRIORITY 3 - Isolation & Cleanup:
- Local account creation over operator reuse (avoid state pollution).
- Are accounts, tokens, and allowances properly cleaned up to avoid state leakage?
- Recommend teardown strategies or fixture scoping improvements.
- Tests should not depend on execution order (avoid brittle assumptions).
PRIORITY 4 - Assertions & Coverage:
- Do tests validate only success/failure, or also assert resulting state?
- Suggest additional assertions that would strengthen correctness (balances, allowances, ownership).
- Cover happy paths AND ...
Files:
tests/integration/query_e2e_test.pytests/integration/contract_call_query_e2e_test.py
🧬 Code graph analysis (7)
examples/contract/contract_execute_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
src/hiero_sdk_python/query/query.py (1)
src/hiero_sdk_python/hbar.py (2)
Hbar(18-213)to_hbars(67-71)
tests/unit/client_test.py (2)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)src/hiero_sdk_python/client/client.py (2)
Client(32-287)set_default_max_query_payment(249-274)
src/hiero_sdk_python/client/client.py (1)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)
examples/contract/contract_call_query.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
tests/integration/contract_call_query_e2e_test.py (1)
src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
tests/unit/query_test.py (2)
src/hiero_sdk_python/query/query.py (4)
set_max_query_payment(96-124)_is_payment_required(414-421)get_cost(281-321)_before_execute(126-157)src/hiero_sdk_python/client/client.py (1)
set_default_max_query_payment(249-274)
🪛 Ruff (0.14.10)
src/hiero_sdk_python/query/query.py
96-96: Use X | Y for type annotations
Convert to X | Y
(UP007)
113-115: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
154-157: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/client_test.py
192-192: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
src/hiero_sdk_python/client/client.py
7-7: typing.List is deprecated, use list instead
(UP035)
249-249: Use X | Y for type annotations
Convert to X | Y
(UP007)
263-265: Avoid specifying long messages outside the exception class
(TRY003)
269-269: Avoid specifying long messages outside the exception class
(TRY003)
tests/integration/query_e2e_test.py
243-243: Redefinition of unused env from line 12
(F811)
255-255: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
259-259: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
tests/unit/query_test.py
251-251: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
254-254: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run-examples
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.10)
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (2)
CHANGELOG.md (1)
90-90: LGTM!The changelog entry clearly documents the new max query payment feature, including both per-query and client-wide default maximum payment methods.
examples/contract/contract_execute_transaction.py (1)
110-120: Reconsider the cost-fetch-and-set pattern for better user guidance.Same concern as in
contract_call_query.py: the current pattern (fetch cost → set max to exact cost → execute) doesn't effectively demonstrate the protection feature and adds unnecessary complexity for users.Consider using one of these simpler alternatives:
Option 1 (Recommended): Set a client-wide default once in
setup_client():def setup_client(): # ... existing setup code ... client.set_default_max_query_payment(Hbar(10)) # Set once for all queries return clientThen execute directly:
result = ( ContractCallQuery() .set_contract_id(contract_id) .set_gas(2000000) .set_function("getMessage") .execute(client) )Option 2: Set a fixed reasonable max per query:
query = ( ContractCallQuery() .set_contract_id(contract_id) .set_gas(2000000) .set_function("getMessage") .set_max_query_payment(Hbar(5)) ) result = query.execute(client)Both approaches are simpler and better demonstrate how the protection feature works.
Based on examples coding guidelines.
Likely an incorrect or invalid review comment.
| query = ( | ||
| ContractCallQuery() | ||
| .set_contract_id(contract_id) | ||
| .set_gas(2000000) | ||
| .set_function( | ||
| "getMessageAndOwner" | ||
| ) # Call the contract's getMessageAndOwner() function | ||
| .execute(client) | ||
| ) | ||
| cost = query.get_cost(client) | ||
| query.set_max_query_payment(cost) | ||
|
|
||
| result = query.execute(client) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reconsider the cost-fetch-and-set pattern for better user guidance.
The current pattern (fetch cost → set max to exact cost → execute) doesn't effectively demonstrate the protection feature and adds unnecessary complexity for users who will copy this example.
Setting the max to exactly the fetched cost means the query will never fail due to cost limits, which defeats the purpose of the feature.
Consider using one of these simpler alternatives:
Option 1 (Recommended): Set a client-wide default once in setup_client():
def setup_client():
"""Initialize and set up the client with operator account"""
network = Network(network_name)
print(f"Connecting to Hedera {network_name} network!")
client = Client(network)
operator_id = AccountId.from_string(os.getenv("OPERATOR_ID", ""))
operator_key = PrivateKey.from_string(os.getenv("OPERATOR_KEY", ""))
client.set_operator(operator_id, operator_key)
client.set_default_max_query_payment(Hbar(10)) # Set once for all queries
print(f"Client set up with operator id {client.operator_account_id}")
return clientThen execute queries directly:
result = (
ContractCallQuery()
.set_contract_id(contract_id)
.set_gas(2000000)
.set_function("getMessageAndOwner")
.execute(client)
)Option 2: Set a fixed reasonable max per query:
query = (
ContractCallQuery()
.set_contract_id(contract_id)
.set_gas(2000000)
.set_function("getMessageAndOwner")
.set_max_query_payment(Hbar(5))
)
result = query.execute(client)Both approaches are simpler and better demonstrate how the protection feature works.
Based on examples coding guidelines: "Examples are copied by users - prioritize explicitness over brevity."
| query = ( | ||
| ContractCallQuery() | ||
| .set_contract_id(contract_id) | ||
| .set_gas(2000000) | ||
| .set_function("getMessage") | ||
| .execute(client) | ||
| ) | ||
|
|
||
| cost = query.get_cost(client) | ||
| query.set_max_query_payment(cost) | ||
|
|
||
| result = query.execute(client) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reconsider the cost-fetch-and-set pattern for better user guidance.
Same concern as in the other contract examples: the current pattern (fetch cost → set max to exact cost → execute) doesn't effectively demonstrate the protection feature and adds unnecessary complexity.
Consider using one of these simpler alternatives:
Option 1 (Recommended): Set a client-wide default once in setup_client():
def setup_client():
# ... existing setup code ...
client.set_default_max_query_payment(Hbar(10))
return clientThen execute directly:
result = (
ContractCallQuery()
.set_contract_id(contract_id)
.set_gas(2000000)
.set_function("getMessage")
.execute(client)
)Option 2: Set a fixed reasonable max per query:
query = (
ContractCallQuery()
.set_contract_id(contract_id)
.set_gas(2000000)
.set_function("getMessage")
.set_max_query_payment(Hbar(5))
)
result = query.execute(client)Both approaches are simpler and better demonstrate how the protection feature works.
Based on examples coding guidelines.
| @pytest.mark.parametrize( | ||
| 'valid_amount,expected', | ||
| [ | ||
| (1, Hbar(1)), | ||
| (0.1, Hbar(0.1)), | ||
| (Decimal('0.1'), Hbar(Decimal('0.1'))), | ||
| (Hbar(1), Hbar(1)), | ||
| (Hbar(0), Hbar(0)) | ||
| ] | ||
| ) | ||
| def test_set_default_max_query_payment_valid_param(valid_amount, expected): | ||
| """Test that set_default_max_query_payment correctly converts various input types to Hbar.""" | ||
| client = Client.for_testnet() | ||
| # by default is 1 hbar before setting it | ||
| assert client.default_max_query_payment == Hbar(1) | ||
| client.set_default_max_query_payment(valid_amount) | ||
| assert client.default_max_query_payment == expected |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add fluent interface assertion and resource cleanup.
Per unit test coding guidelines (PRIORITY 1): "Assert fluent setters return self". The test verifies value conversion but doesn't assert that set_default_max_query_payment returns the client instance for method chaining.
Additionally, the test should close the client for proper resource cleanup.
🔧 Proposed fix
def test_set_default_max_query_payment_valid_param(valid_amount, expected):
"""Test that set_default_max_query_payment correctly converts various input types to Hbar."""
client = Client.for_testnet()
# by default is 1 hbar before setting it
assert client.default_max_query_payment == Hbar(1)
- client.set_default_max_query_payment(valid_amount)
+ result = client.set_default_max_query_payment(valid_amount)
+ assert result is client, "set_default_max_query_payment should return self for method chaining"
assert client.default_max_query_payment == expected
+ client.close()As per coding guidelines: "Assert fluent setters return self".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.parametrize( | |
| 'valid_amount,expected', | |
| [ | |
| (1, Hbar(1)), | |
| (0.1, Hbar(0.1)), | |
| (Decimal('0.1'), Hbar(Decimal('0.1'))), | |
| (Hbar(1), Hbar(1)), | |
| (Hbar(0), Hbar(0)) | |
| ] | |
| ) | |
| def test_set_default_max_query_payment_valid_param(valid_amount, expected): | |
| """Test that set_default_max_query_payment correctly converts various input types to Hbar.""" | |
| client = Client.for_testnet() | |
| # by default is 1 hbar before setting it | |
| assert client.default_max_query_payment == Hbar(1) | |
| client.set_default_max_query_payment(valid_amount) | |
| assert client.default_max_query_payment == expected | |
| @pytest.mark.parametrize( | |
| 'valid_amount,expected', | |
| [ | |
| (1, Hbar(1)), | |
| (0.1, Hbar(0.1)), | |
| (Decimal('0.1'), Hbar(Decimal('0.1'))), | |
| (Hbar(1), Hbar(1)), | |
| (Hbar(0), Hbar(0)) | |
| ] | |
| ) | |
| def test_set_default_max_query_payment_valid_param(valid_amount, expected): | |
| """Test that set_default_max_query_payment correctly converts various input types to Hbar.""" | |
| client = Client.for_testnet() | |
| # by default is 1 hbar before setting it | |
| assert client.default_max_query_payment == Hbar(1) | |
| result = client.set_default_max_query_payment(valid_amount) | |
| assert result is client, "set_default_max_query_payment should return self for method chaining" | |
| assert client.default_max_query_payment == expected | |
| client.close() |
🧰 Tools
🪛 Ruff (0.14.10)
192-192: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
| @pytest.mark.parametrize( | ||
| 'negative_amount', | ||
| [-1, -0.1, Decimal('-0.1'), Decimal('-1')] | ||
| ) | ||
| def test_set_default_max_query_payment_negative_value(negative_amount): | ||
| """Test set_default_max_query_payment for negative amount values.""" | ||
| client = Client.for_testnet() | ||
|
|
||
| with pytest.raises(ValueError, match=f"max_query_payment must be non-negative, got {negative_amount}"): | ||
| client.set_default_max_query_payment(negative_amount) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add resource cleanup.
The test creates a client but doesn't close it. Add cleanup for consistency with other tests in this file.
🔧 Proposed fix
def test_set_default_max_query_payment_negative_value(negative_amount):
"""Test set_default_max_query_payment for negative amount values."""
client = Client.for_testnet()
with pytest.raises(ValueError, match=f"max_query_payment must be non-negative, got {negative_amount}"):
client.set_default_max_query_payment(negative_amount)
+
+ client.close()| @pytest.mark.parametrize( | ||
| 'invalid_amount', | ||
| ['1', 'abc', None, object()] | ||
| ) | ||
| def test_set_default_max_query_payment_invalid_param(invalid_amount): | ||
| """Test that set_default_max_query_payment raise error for invalid param.""" | ||
| client = Client.for_testnet() | ||
|
|
||
| with pytest.raises(TypeError, match=( | ||
| f"max_query_payment must be int, float, Decimal, or Hbar, " | ||
| f"got {type(invalid_amount).__name__}" | ||
| )): | ||
| client.set_default_max_query_payment(invalid_amount) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add resource cleanup.
Same cleanup consideration as the negative value tests.
🔧 Proposed fix
def test_set_default_max_query_payment_invalid_param(invalid_amount):
"""Test that set_default_max_query_payment raise error for invalid param."""
client = Client.for_testnet()
with pytest.raises(TypeError, match=(
f"max_query_payment must be int, float, Decimal, or Hbar, "
f"got {type(invalid_amount).__name__}"
)):
client.set_default_max_query_payment(invalid_amount)
+
+ client.close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.parametrize( | |
| 'invalid_amount', | |
| ['1', 'abc', None, object()] | |
| ) | |
| def test_set_default_max_query_payment_invalid_param(invalid_amount): | |
| """Test that set_default_max_query_payment raise error for invalid param.""" | |
| client = Client.for_testnet() | |
| with pytest.raises(TypeError, match=( | |
| f"max_query_payment must be int, float, Decimal, or Hbar, " | |
| f"got {type(invalid_amount).__name__}" | |
| )): | |
| client.set_default_max_query_payment(invalid_amount) | |
| @pytest.mark.parametrize( | |
| 'invalid_amount', | |
| ['1', 'abc', None, object()] | |
| ) | |
| def test_set_default_max_query_payment_invalid_param(invalid_amount): | |
| """Test that set_default_max_query_payment raise error for invalid param.""" | |
| client = Client.for_testnet() | |
| with pytest.raises(TypeError, match=( | |
| f"max_query_payment must be int, float, Decimal, or Hbar, " | |
| f"got {type(invalid_amount).__name__}" | |
| )): | |
| client.set_default_max_query_payment(invalid_amount) | |
| client.close() |
| assert query._is_payment_required() == True | ||
| # Verify that payment-requiring query also defaults to requiring payment | ||
| assert query_requires_payment._is_payment_required() == True | ||
| assert query_requires_payment._is_payment_required() == True |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use direct truth check instead of equality comparison to True.
Per Python best practices and static analysis (E712), use direct truth check for boolean returns.
♻️ Proposed fix
- assert query_requires_payment._is_payment_required() == True
+ assert query_requires_payment._is_payment_required()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert query_requires_payment._is_payment_required() == True | |
| assert query_requires_payment._is_payment_required() |
🧰 Tools
🪛 Ruff (0.14.10)
251-251: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
| @pytest.mark.parametrize( | ||
| 'valid_amount,expected', | ||
| [ | ||
| (1, Hbar(1)), | ||
| (0.1, Hbar(0.1)), | ||
| (Decimal('0.1'), Hbar(Decimal('0.1'))), | ||
| (Hbar(1), Hbar(1)), | ||
| (Hbar(0), Hbar(0)) | ||
| ] | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use tuple for pytest.mark.parametrize first argument.
Per pytest conventions (PT006), the first argument should be a tuple of parameter names.
♻️ Proposed fix
@pytest.mark.parametrize(
- 'valid_amount,expected',
+ ('valid_amount', 'expected'),
[
(1, Hbar(1)),
(0.1, Hbar(0.1)),
(Decimal('0.1'), Hbar(Decimal('0.1'))),
(Hbar(1), Hbar(1)),
(Hbar(0), Hbar(0))
]
)Apply the same fix to lines 270-272 and 279-281.
🧰 Tools
🪛 Ruff (0.14.10)
254-254: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
| def test_set_max_query_payment_valid_param(query, valid_amount, expected): | ||
| """Test that set_max_query_payment correctly converts various input types to Hbar.""" | ||
| # by default is none before setting it | ||
| assert query.max_query_payment is None | ||
| query.set_max_query_payment(valid_amount) | ||
| assert query.max_query_payment == expected |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add assertion for fluent interface (method chaining).
Per unit test coding guidelines: "Assert fluent setters return self". The test verifies value conversion but doesn't validate that set_max_query_payment returns the query instance for method chaining.
♻️ Proposed fix
def test_set_max_query_payment_valid_param(query, valid_amount, expected):
"""Test that set_max_query_payment correctly converts various input types to Hbar."""
# by default is none before setting it
assert query.max_query_payment is None
- query.set_max_query_payment(valid_amount)
+ result = query.set_max_query_payment(valid_amount)
+ assert result is query, "set_max_query_payment should return self for method chaining"
assert query.max_query_payment == expected| query_requires_payment.set_max_query_payment(2) | ||
| assert query_requires_payment.max_query_payment == Hbar(2) | ||
|
|
||
| # mock the get_cost to return 2 hbar as required paymnet |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fix typos in comments.
Minor typos: "resturn" should be "return", "paymnet" should be "payment".
♻️ Proposed fix
- # mock the get_cost to return 2 hbar as required paymnet
+ # mock the get_cost to return 2 hbar as required paymentAlso applies to line 349.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # mock the get_cost to return 2 hbar as required paymnet | |
| # mock the get_cost to return 2 hbar as required payment |
2 participants
Description:
This PR introduces support for maximum query payment limits for
Queryclass by:max_query_payment fieldto the Query base class.set_max_query_payment(...)max_query_payment, if set, ordefault_max_query_paymentas a fallbackChanges Made:
max_query_paymentsupport to the Query base class.set_max_query_payment()to allow setting a per-query maximum payment.DEFAULT_MAX_QUERY_PAYMENT = Hbar(1)to client.set_default_max_query_payment()on Client to define a client-wide default maximum query payment.Related issue(s):
Fixes #1346
Notes for reviewer:
Checklist