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 2 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.66% 93.70% +0.03%
==========================================
Files 144 144
Lines 9350 9376 +26
==========================================
+ Hits 8758 8786 +28
+ Misses 592 590 -2 🚀 New features to boost your workflow:
|
WalkthroughAdds a client-level default max transaction fee and per-transaction fee API: introduces 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: new 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(2)
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 | ✅ 5✅ Passed checks (5 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") |
| @pytest.mark.parametrize("value", [0, 1, 100_000_000, Hbar(1)]) | ||
| def test_set_transaction_fee(value): | ||
| """set_transaction_fee() stores valid integer fees.""" | ||
| transaction = TransferTransaction() | ||
|
|
||
| returned = transaction.set_transaction_fee(value) | ||
|
|
||
| assert returned is transaction | ||
| assert transaction.transaction_fee == value |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor docstring inaccuracy: test accepts both int and Hbar.
The docstring says "stores valid integer fees" but the test parametrization includes Hbar(1). Consider updating the docstring for accuracy.
📝 Suggested docstring fix
`@pytest.mark.parametrize`("value", [0, 1, 100_000_000, Hbar(1)])
def test_set_transaction_fee(value):
- """set_transaction_fee() stores valid integer fees."""
+ """set_transaction_fee() stores valid int or Hbar fees."""
transaction = TransferTransaction()📝 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("value", [0, 1, 100_000_000, Hbar(1)]) | |
| def test_set_transaction_fee(value): | |
| """set_transaction_fee() stores valid integer fees.""" | |
| transaction = TransferTransaction() | |
| returned = transaction.set_transaction_fee(value) | |
| assert returned is transaction | |
| assert transaction.transaction_fee == value | |
| `@pytest.mark.parametrize`("value", [0, 1, 100_000_000, Hbar(1)]) | |
| def test_set_transaction_fee(value): | |
| """set_transaction_fee() stores valid int or Hbar fees.""" | |
| transaction = TransferTransaction() | |
| returned = transaction.set_transaction_fee(value) | |
| assert returned is transaction | |
| assert transaction.transaction_fee == value |
|
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
3 participants
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