feat(client): add global max transaction fee configuration#2332
feat(client): add global max transaction fee configuration#2332tech0priyanshu wants to merge 5 commits into
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughAdds a client-level ChangesClient and Transaction Max Fee Configuration
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Client
participant Transaction
participant TransactionBody
Developer->>Client: set_max_transaction_fee value
Client->>Client: validate input and convert to Hbar
Developer->>Transaction: set_max_transaction_fee value
Transaction->>Transaction: validate input and set transaction_fee
Developer->>Transaction: freeze_with client
alt transaction_fee is set
Transaction->>TransactionBody: use explicit transaction fee
else transaction_fee is not set
Transaction->>Client: read default_max_transaction_fee
alt client default exists
Transaction->>TransactionBody: use client default fee
else no client default
Transaction->>TransactionBody: use class default fee
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
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: 5f52c8b9-6025-4004-85ec-a0350465b3be
📒 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
| def test_fee_resolution_transaction_precedence(mock_client): | ||
| """Transaction fee explicitly set should take precedence over client default.""" | ||
| tx = TransferTransaction() | ||
| tx.set_max_transaction_fee(Hbar(10)) | ||
|
|
||
| # client has different default | ||
| mock_client.set_max_transaction_fee(Hbar(5)) | ||
|
|
||
| tx.freeze_with(mock_client) | ||
|
|
||
| assert tx.transaction_fee == Hbar(10) | ||
|
|
||
|
|
||
| def test_fee_resolution_client_default_used_when_transaction_missing(mock_client): | ||
| """When transaction fee is not set, client.default_max_transaction_fee should be used.""" | ||
| tx = TransferTransaction() | ||
| # leave tx.transaction_fee as None | ||
|
|
||
| mock_client.set_max_transaction_fee(Hbar(7)) | ||
|
|
||
| tx.freeze_with(mock_client) | ||
|
|
||
| assert tx.transaction_fee == Hbar(7) | ||
|
|
||
|
|
||
| def test_fee_resolution_falls_back_to_transaction_default(mock_client): | ||
| """When neither transaction nor client provide a fee, fallback to transaction default Hbar(1).""" | ||
| tx = TransferTransaction() | ||
| tx.set_transaction_id(TransactionId.generate(AccountId.from_string("0.0.1234"))) | ||
| # Ensure client default is None | ||
| mock_client.default_max_transaction_fee = None | ||
|
|
||
| tx.freeze_with(mock_client) | ||
|
|
||
| assert tx.transaction_fee == 100000000 # Default fee for TransferTransaction |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test coverage for fee resolution in single-node and multi-node paths.
The current tests (lines 728-762) only verify fee resolution when freeze_with(mock_client) uses the client's network nodes. Missing coverage: fee resolution when node_account_id or node_account_ids are explicitly set before freeze_with().
These additional test scenarios would catch the major issue flagged in transaction.py where the client's default_max_transaction_fee is bypassed in those code paths.
🧪 Proposed additional tests
def test_fee_resolution_with_explicit_node_account_id(mock_client):
"""Fee resolution should work when node_account_id is explicitly set."""
tx = TransferTransaction()
tx.node_account_id = AccountId(0, 0, 3)
mock_client.set_max_transaction_fee(Hbar(7))
tx.freeze_with(mock_client)
# Should use client default, not transaction default
assert tx.transaction_fee == Hbar(7)
def test_fee_resolution_with_explicit_node_account_ids(mock_client):
"""Fee resolution should work when node_account_ids list is explicitly set."""
tx = TransferTransaction()
tx.node_account_ids = [AccountId(0, 0, 3), AccountId(0, 0, 4)]
mock_client.set_max_transaction_fee(Hbar(8))
tx.freeze_with(mock_client)
# Should use client default, not transaction default
assert tx.transaction_fee == Hbar(8)As per coding guidelines: "PRIORITY 3 - Comprehensive Coverage: Cover happy paths AND unhappy paths/edge cases."
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #2332 +/- ##
=======================================
Coverage 95.02% 95.03%
=======================================
Files 163 163
Lines 10473 10494 +21
=======================================
+ Hits 9952 9973 +21
Misses 521 521 🚀 New features to boost your workflow:
|
|
Hi, this is WorkflowBot.
|
612e7a1 to
c7d3316
Compare
|
@tech0priyanshu is this ready for review |
|
need a bit time @tech0priyanshu |
749aad6 to
70637d1
Compare
|
pls review @manishdait @exploreriii |
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: 190c792b-bf8e-4549-9c44-2d9c78874f68
📒 Files selected for processing (7)
src/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/transaction/transaction.pytests/integration/account_update_transaction_e2e_test.pytests/unit/client_test.pytests/unit/fee_estimate_query_test.pytests/unit/file_append_transaction_test.pytests/unit/transaction_freeze_and_bytes_test.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 190c792b-bf8e-4549-9c44-2d9c78874f68
📒 Files selected for processing (7)
src/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/transaction/transaction.pytests/integration/account_update_transaction_e2e_test.pytests/unit/client_test.pytests/unit/fee_estimate_query_test.pytests/unit/file_append_transaction_test.pytests/unit/transaction_freeze_and_bytes_test.py
🛑 Comments failed to post (1)
src/hiero_sdk_python/transaction/transaction.py (1)
489-493:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winExplicit zero fee is overwritten by default fee.
Line 489 and Line 517 use truthy fallback (
self._transaction_fee or self._default_transaction_fee). If fee is explicitly set to0, serialization silently falls back to the default fee, so zero-fee configuration is ignored.💡 Suggested fix
- fee = self._transaction_fee or self._default_transaction_fee + fee = self._transaction_fee if self._transaction_fee is not None else self._default_transaction_fee if hasattr(fee, "to_tinybars"): transaction_body.transactionFee = int(fee.to_tinybars()) else: transaction_body.transactionFee = int(fee) ... - fee = self._transaction_fee or self._default_transaction_fee + fee = self._transaction_fee if self._transaction_fee is not None else self._default_transaction_fee if hasattr(fee, "to_tinybars"): schedulable_body.transactionFee = int(fee.to_tinybars()) else: schedulable_body.transactionFee = int(fee)Also applies to: 517-521
|
@tech0priyanshu Is this ready for review again? |
|
Yeah 👍 |
| # 1. Explicit transaction fee (self.transaction_fee) | ||
| # 2. Client default_max_transaction_fee | ||
| # 3. Transaction class default (_default_transaction_fee) | ||
| if self.transaction_fee is None: |
There was a problem hiding this comment.
Is it intentional that resolving the client default fee permanently mutates transaction_fee during freeze?
I believe a change may be required here as per issue.
exploreriii
left a comment
There was a problem hiding this comment.
Hi @tech0priyanshu
Great work but we need to check for some inconsistencies as right now some parts can be handling 1 hbar and others 1 tinybar then should be looking pretty good. Really check up the typing
| transaction_body, signed_transaction.bodyBytes, signed_transaction.sigMap | ||
| ) | ||
|
|
||
| def set_max_transaction_fee(self, max_transaction_fee): |
There was a problem hiding this comment.
Some of the logic here is quite complex, and duplicated elsewhere, which has also caused some typing curiosities
why not extract the fee calculation and place it e.g. in the hbar file?
self.default_max_transaction_fee = _coerce_fee(max_transaction_fee)
Then create
def _coerce_fee(value: int | float | Decimal | Hbar) -> Hbar:
and call that from the multiple points
|
|
||
| def set_max_transaction_fee( |
There was a problem hiding this comment.
ideally can just call that same thing from the hbar class
|
@exploreriii @aceppaluni Sorry I was busy last. But yeah, i try to update asap |
4b94571 to
2a22053
Compare
|
Hello, this is the OfficeHourBot. This is a reminder that the Hiero Python SDK Office Hours will begin in approximately 2 hours and 28 minutes (14:00 UTC). This session provides an opportunity to ask questions regarding this Pull Request. Details:
Disclaimer: This is an automated reminder. Please verify the schedule here for any changes. From, |
|
Hi @tech0priyanshu let us know please when this is ready to review again, thanks |
2a22053 to
ed12851
Compare
|
Hi @tech0priyanshu, This pull request has had no commit activity for 10 days. Are you still working on it?
If you're no longer working on this, please comment Reach out on discord or join our office hours if you need assistance. From the Python SDK Team |
Signed-off-by: tech0priyanshu <priyanshuyadv101106@gmail.com>
Signed-off-by: tech0priyanshu <priyanshuyadv101106@gmail.com>
Signed-off-by: tech0priyanshu <priyanshuyadv101106@gmail.com>
Signed-off-by: tech0priyanshu <priyanshuyadv101106@gmail.com>
ed12851 to
f3ceca8
Compare
Description:
Adds support for configuring a global maximum transaction fee at the client level.
Related issue(s):
Fixes #2000
Notes for reviewer:
Checklist