feat: add global set_default_max_transaction_fee to Client#2026
feat: add global set_default_max_transaction_fee to Client#2026AntonioCeppellini wants to merge 6 commits intohiero-ledger:mainfrom
Conversation
Signed-off-by: Antonio Ceppellini <antonio.ceppellini@gmail.com>
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #2026 +/- ##
==========================================
+ Coverage 93.68% 93.70% +0.01%
==========================================
Files 144 144
Lines 9350 9375 +25
==========================================
+ Hits 8760 8785 +25
Misses 590 590 🚀 New features to boost your workflow:
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughAdds a client-level default max transaction fee and per-transaction fee API: adds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant Transaction
participant FreezeLogic
User->>Client: set_default_max_transaction_fee(Hbar(2))
Client->>Client: validate Hbar / store default_max_transaction_fee
User->>Transaction: create Transaction (transaction_fee = None)
User->>Transaction: freeze_with(client)
activate FreezeLogic
FreezeLogic->>Transaction: is transaction_fee None?
alt transaction_fee is None
FreezeLogic->>Client: read default_max_transaction_fee
alt client.default_max_transaction_fee != None
Client-->>FreezeLogic: Hbar(...)
FreezeLogic->>Transaction: set transaction_fee = client default
else
FreezeLogic->>Transaction: set transaction_fee = _default_transaction_fee (Hbar(2))
end
else
FreezeLogic->>Transaction: keep existing transaction_fee
end
deactivate FreezeLogic
Transaction-->>User: frozen transaction (transaction_fee resolved)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3eaa559-7c21-4dd5-b3de-06cc6e06502c
📒 Files selected for processing (5)
CHANGELOG.mdsrc/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/transaction/transaction.pytests/unit/client_test.pytests/unit/transaction_freeze_and_bytes_test.py
|
Hi, this is WorkflowBot.
|
exploreriii
added
status: needs committer review
| self.transaction_id = transaction_id | ||
| return self | ||
|
|
||
| def set_transaction_fee(self, transaction_fee) -> "Transaction": |
There was a problem hiding this comment.
it will be nice if we also accept the Hbar and int both as input
|
Hey @AntonioCeppellini, great work on this!, I think this should be resolved once the requested changes are applied |
|
@manishdait thank you! new commit incoming :D |
Signed-off-by: Antonio Ceppellini <antonio.ceppellini@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/unit/client_test.py (1)
497-502: 🧹 Nitpick | 🔵 TrivialAdd boundary test for
Hbar(0)and remove unnecessary f-string prefix.Per unit test guidelines (PRIORITY 3), boundary conditions should be tested.
Hbar(0)is a valid edge case that should be accepted. Also, the f-string prefix is unnecessary since there's no interpolation.💚 Proposed additions
def test_set_invalid_value_default_max_transaction_fee(): """Test that set invalid value with set_max_transaction fee.""" client = Client.for_testnet() - with pytest.raises(ValueError, match=f"default_max_transaction_fee must be >= 0"): + with pytest.raises(ValueError, match="default_max_transaction_fee must be >= 0"): client.set_default_max_transaction_fee(Hbar(-1)) + + +def test_set_default_max_transaction_fee_zero_is_valid(): + """Test that set_default_max_transaction_fee accepts zero as a valid boundary value.""" + client = Client.for_testnet() + + returned = client.set_default_max_transaction_fee(Hbar(0)) + + assert returned is client + assert client.default_max_transaction_fee == Hbar(0) + + client.close()
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c5f6e36f-e76a-4326-bca6-646bb642a1a4
📒 Files selected for processing (4)
src/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/transaction/transaction.pytests/unit/client_test.pytests/unit/transaction_freeze_and_bytes_test.py
| if self.transaction_fee is None: | ||
| if client.default_max_transaction_fee is not None: | ||
| self.transaction_fee = client.default_max_transaction_fee | ||
| else: | ||
| self.transaction_fee = self._default_transaction_fee |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify FileAppendTransaction.freeze_with() does not call super()
rg -n "def freeze_with" src/hiero_sdk_python/file/file_append_transaction.py -A 50 | head -60
# Check if super().freeze_with is called
rg -n "super\(\)\.freeze_with" src/hiero_sdk_python/file/file_append_transaction.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 2531
🏁 Script executed:
# Read transaction.py to see the full freeze_with implementation around lines 317-321
sed -n '300,350p' src/hiero_sdk_python/transaction/transaction.py
# Also check FileAppendTransaction's __init__ or any fee-related logic
rg -n "transaction_fee\|_default_transaction_fee" src/hiero_sdk_python/file/file_append_transaction.py -B 2 -A 2Repository: hiero-ledger/hiero-sdk-python
Length of output: 2052
🏁 Script executed:
# Check FileAppendTransaction class definition and inheritance
rg -n "^class FileAppendTransaction" src/hiero_sdk_python/file/file_append_transaction.py -A 3
# Check __init__ method for fee initialization
rg -n "def __init__" src/hiero_sdk_python/file/file_append_transaction.py -A 30 | head -50
# Check if FileAppendTransaction or parent Transaction sets any default fee
rg -n "transaction_fee" src/hiero_sdk_python/transaction/transaction.py | head -20Repository: hiero-ledger/hiero-sdk-python
Length of output: 3278
🏁 Script executed:
# Check FileAppendTransaction.build_transaction_body() implementation
rg -n "def build_transaction_body" src/hiero_sdk_python/file/file_append_transaction.py -A 30
# Check if FileAppendTransaction has any fee-related logic
rg -n "transactionFee\|transaction_fee" src/hiero_sdk_python/file/file_append_transaction.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 1399
🏁 Script executed:
# Check Transaction.build_base_transaction_body() to see how it handles transaction_fee
rg -n "def build_base_transaction_body" src/hiero_sdk_python/transaction/transaction.py -A 20Repository: hiero-ledger/hiero-sdk-python
Length of output: 973
🏁 Script executed:
# Get more lines from build_base_transaction_body to see fee handling
sed -n '491,550p' src/hiero_sdk_python/transaction/transaction.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 2430
🏁 Script executed:
# Check for FileAppendTransaction tests, especially fee-related
find . -name "*test*file*append*" -o -name "*file_append*test*" 2>/dev/null | head -10
# Also search for any test files that test FileAppendTransaction
rg -l "FileAppendTransaction" --type py | grep -i testRepository: hiero-ledger/hiero-sdk-python
Length of output: 270
🏁 Script executed:
# Check unit tests for FileAppendTransaction, especially fee-related tests
rg -n "transaction_fee\|default.*fee\|freeze_with" tests/unit/file_append_transaction_test.py | head -30
# Get overall structure of the test file
wc -l tests/unit/file_append_transaction_test.py
head -100 tests/unit/file_append_transaction_test.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 3784
🏁 Script executed:
# Search for more fee-related tests in the file
rg -n "default.*fee\|transaction_fee" tests/unit/file_append_transaction_test.py -B 3 -A 10
# Also check integration tests
rg -n "default.*fee\|transaction_fee" tests/integration/file_append_transaction_e2e_test.py -B 3 -A 10Repository: hiero-ledger/hiero-sdk-python
Length of output: 55
🏁 Script executed:
# Check for any comments or documentation in FileAppendTransaction about why freeze_with is overridden
sed -n '270,330p' src/hiero_sdk_python/file/file_append_transaction.py | head -70
# Check if there are any other Transaction subclasses that override freeze_with
rg -n "def freeze_with" src/hiero_sdk_python --type py | grep -v "file_append_transaction"Repository: hiero-ledger/hiero-sdk-python
Length of output: 2911
🏁 Script executed:
# Check if TokenCreateTransaction.freeze_with calls super()
rg -n "def freeze_with" src/hiero_sdk_python/tokens/token_create_transaction.py -A 30 | head -40
# Check TopicMessageSubmitTransaction.freeze_with
rg -n "def freeze_with" src/hiero_sdk_python/consensus/topic_message_submit_transaction.py -A 30 | head -40Repository: hiero-ledger/hiero-sdk-python
Length of output: 2770
🏁 Script executed:
# Let me verify the exact difference in structure between TopicMessageSubmitTransaction and FileAppendTransaction
# to understand why one calls super() and the other doesn't
# Check the full context of how TopicMessageSubmitTransaction handles the chunking + super call
sed -n '258,287p' src/hiero_sdk_python/consensus/topic_message_submit_transaction.py
# Check if there are any comments in FileAppendTransaction explaining the override design
sed -n '265,275p' src/hiero_sdk_python/file/file_append_transaction.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 1752
FileAppendTransaction.freeze_with() bypasses fee auto-population.
The fee auto-population logic at lines 317-321 (client default fallback) is skipped for FileAppendTransaction because its freeze_with() override manually builds all transaction bodies without calling super().freeze_with(client). This causes all chunks to use the hardcoded _default_transaction_fee (Hbar 5) and ignore client.default_max_transaction_fee.
In contrast, TopicMessageSubmitTransaction (another chunked transaction) correctly performs chunk setup then delegates to super().freeze_with(client) to apply the fee logic. FileAppendTransaction should follow this pattern.
| if isinstance(transaction_fee, int): | ||
| if transaction_fee < 0: | ||
| raise ValueError("transaction_fee must be >= 0") | ||
|
|
||
| if isinstance(transaction_fee, Hbar): | ||
| if transaction_fee < Hbar(0): | ||
| raise ValueError("transaction_fee must be >= 0") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify negative value validation by consolidating checks.
The separate if isinstance(..., int) and if isinstance(..., Hbar) blocks for negative value checking can be simplified. Additionally, bool is a subclass of int in Python, so True/False would pass the int check and potentially be stored.
♻️ Proposed simplification
+ if isinstance(transaction_fee, bool):
+ raise TypeError(
+ f"transaction_fee must be an int or Hbar, got {type(transaction_fee).__name__}"
+ )
+
if isinstance(transaction_fee, int):
if transaction_fee < 0:
raise ValueError("transaction_fee must be >= 0")
-
- if isinstance(transaction_fee, Hbar):
- if transaction_fee < Hbar(0):
+ elif isinstance(transaction_fee, Hbar):
+ if transaction_fee.to_tinybars() < 0:
raise ValueError("transaction_fee must be >= 0")📝 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 isinstance(transaction_fee, int): | |
| if transaction_fee < 0: | |
| raise ValueError("transaction_fee must be >= 0") | |
| if isinstance(transaction_fee, Hbar): | |
| if transaction_fee < Hbar(0): | |
| raise ValueError("transaction_fee must be >= 0") | |
| if isinstance(transaction_fee, bool): | |
| raise TypeError( | |
| f"transaction_fee must be an int or Hbar, got {type(transaction_fee).__name__}" | |
| ) | |
| if isinstance(transaction_fee, int): | |
| if transaction_fee < 0: | |
| raise ValueError("transaction_fee must be >= 0") | |
| elif isinstance(transaction_fee, Hbar): | |
| if transaction_fee.to_tinybars() < 0: | |
| raise ValueError("transaction_fee must be >= 0") |
|
In Proposed change: receipt = tx.execute(env.client)
assert receipt.status == ResponseCode.INSUFFICIENT_TX_FEE, (
f"Expected INSUFFICIENT_TX_FEE but got {ResponseCode(receipt.status).name}"
)
# Replace this with
with pytest.raises(PrecheckError) as e:
tx.execute(env.client)
assert e.value.status == ResponseCode.INSUFFICIENT_TX_FEERight now, the test is relying on |
|
Also We can simplify this by removing |
manishdait
added
status: needs developer revision
Signed-off-by: Antonio Ceppellini <antonio.ceppellini@gmail.com>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 2 medium |
| Documentation | 1 minor |
| ErrorProne | 2 high |
| CodeStyle | 5 minor |
| Comprehensibility | 2 minor |
🟢 Metrics 11 complexity
Metric Results Complexity 11
TIP This summary will be updated as you push new changes. Give us feedback
There was a problem hiding this comment.
Pull request overview
This PR adds a client-level default maximum transaction fee configuration, allowing transactions to inherit a global fee unless explicitly overridden per-transaction.
Changes:
- Add
Client.default_max_transaction_feeand aClient.set_default_max_transaction_fee()setter. - Update
Transaction.freeze_with()to resolvetransaction_feefrom the client default (or fall back to a transaction default ofHbar(2)). - Add
Transaction.set_transaction_fee()plus unit/integration test coverage for the new behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/hiero_sdk_python/client/client.py |
Introduces default_max_transaction_fee field and setter on Client. |
src/hiero_sdk_python/transaction/transaction.py |
Adds fee type alias, updates fee defaults/resolution in freeze_with(), and introduces set_transaction_fee(). |
tests/unit/client_test.py |
Adds unit tests for Client.set_default_max_transaction_fee(). |
tests/unit/transaction_freeze_and_bytes_test.py |
Adds unit tests for Transaction.set_transaction_fee() and freeze_with() fee resolution. |
tests/integration/account_update_transaction_e2e_test.py |
Updates insufficient-fee integration test to use set_transaction_fee() and assert PrecheckError. |
CHANGELOG.md |
Adds an “Added” entry documenting the new client-level fee setter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| self._require_not_frozen() | ||
|
|
||
| if not isinstance(transaction_fee, TransactionFee): |
There was a problem hiding this comment.
set_transaction_fee() uses isinstance(transaction_fee, TransactionFee), but TransactionFee is a typing.Union alias, which will raise TypeError at runtime (subscripted generics/typing unions can't be used with isinstance). Validate with isinstance(transaction_fee, (int, Hbar)) (or define TransactionFee = int | Hbar and use a tuple for the runtime check).
| if not isinstance(transaction_fee, TransactionFee): | |
| if not isinstance(transaction_fee, (int, Hbar)): |
| import math | ||
| from typing import Literal, Optional, overload, Union |
There was a problem hiding this comment.
math is imported but not used anywhere in this module, which will fail linting/CI in many configurations. Remove the unused import (and also trim the trailing whitespace in the from typing ... Union import line).
| import math | |
| from typing import Literal, Optional, overload, Union | |
| from typing import Literal, Optional, overload, Union |
| def set_transaction_fee(self, transaction_fee: TransactionFee) -> "Transaction": | ||
| """ | ||
| Sets the transaction fee for the transaction | ||
|
|
||
| Args: | ||
| transaction_fee (TransactionFee): The transaction fee to set. | ||
|
|
||
| Returns: | ||
| Transaction: The current transaction instance for method chaining. |
There was a problem hiding this comment.
set_transaction_fee() accepts 0 (and tests cover it), but fee selection later uses truthiness (self.transaction_fee or self._default_transaction_fee). That means an explicitly-set 0 fee will be ignored and replaced with the default. Use an explicit is None check when selecting the fee so 0 remains a valid override.
| setted_default_max_transaction_fee = Hbar(2) | ||
| client.set_default_max_transaction_fee(setted_default_max_transaction_fee) | ||
| assert client.default_max_transaction_fee == setted_default_max_transaction_fee |
There was a problem hiding this comment.
Variable name setted_default_max_transaction_fee is grammatically incorrect and unclear. Rename to something like default_max_transaction_fee or new_default_max_transaction_fee for readability.
| setted_default_max_transaction_fee = Hbar(2) | |
| client.set_default_max_transaction_fee(setted_default_max_transaction_fee) | |
| assert client.default_max_transaction_fee == setted_default_max_transaction_fee | |
| default_max_transaction_fee = Hbar(2) | |
| client.set_default_max_transaction_fee(default_max_transaction_fee) | |
| assert client.default_max_transaction_fee == default_max_transaction_fee |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a117658a-5bd4-4149-a336-3f806e3d1963
📒 Files selected for processing (1)
tests/integration/account_update_transaction_e2e_test.py
| def test_integration_account_update_transaction_with_max_automatic_token_associations( | ||
| env, | ||
| ): |
There was a problem hiding this comment.
Resolve the F811 env redefinition warning on this changed test signature.
The parameter name collides with the imported symbol name (env), which is triggering static analysis noise on this line.
Proposed lint-safe fix
-from tests.integration.utils import env
+from tests.integration.utils import env as _env_fixture # noqa: F401🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 343-343: tests/integration/account_update_transaction_e2e_test.py#L343
Redefinition of unused env from line 17 (F811)
[warning] 343-343: tests/integration/account_update_transaction_e2e_test.py#L343
redefinition of unused 'env' from line 17 (F811)
Signed-off-by: Antonio Ceppellini <antonio.ceppellini@gmail.com>
Signed-off-by: Antonio Ceppellini <antonio.ceppellini@gmail.com>
Description:
This PR introduces a global
default_max_transaction_feeat theClientlevel.Add support for ...
default_max_transaction_feein client.py default value asNoneset_default_max_transaction_fee_default_transaction_feeto be 2Hbar in thetransaction.pyfreeze_with()intransaction.pyto resolvemax_transaction_feevalueset_transaction_fee()intransaction.pyto set the value ofself.transaction_feeRelated issue(s):
Fixes #2000
Checklist