feat: Add Hip 1261 Implementation#2019
Conversation
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
|
Hi, this is WorkflowBot.
|
|
I have a version of execute where the tests do pass. I am happy to share this version if needed. :) |
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
manishdait
left a comment
manishdait
left a comment
There was a problem hiding this comment.
Great work @aceppaluni, left a few suggestions.
| mode: FeeEstimateMode | ||
| network_fee: NetworkFee | ||
| node_fee: FeeEstimate | ||
| service_fee: FeeEstimate |
There was a problem hiding this comment.
these fields may be absent in the response better to mark them as the Optional[None]:
network_fee: Optional[NetworkFee] = None
node_fee: Optional[FeeEstimate] = None
service_fee: Optional[FeeEstimate] = None|
|
||
| @dataclass(frozen=True) | ||
| class FeeExtra: | ||
| name: str |
There was a problem hiding this comment.
This field may not always be present in the response
| name: str | |
| name: Optional[str] = None |
| def __init__(self): | ||
| self._mode: Optional[FeeEstimateMode] = None | ||
| self._transaction: Optional["Transaction"] = None | ||
|
|
There was a problem hiding this comment.
consider adding query level max_attempts and max_backoff configuration and make them configurable by adding setters
| network_multiplier = None | ||
| notes = [] | ||
|
|
||
| max_retries = getattr(client, "max_retries", 3) |
There was a problem hiding this comment.
we can remove this and use the max_attempts instead
| max_retries = getattr(client, "max_retries", 3) | |
| if self.attempts is None: | |
| self.max_attempts = client.max_attempts |
|
|
||
| tx_bytes = tx.to_bytes() | ||
|
|
||
| for attempt in range(max_retries): |
There was a problem hiding this comment.
| for attempt in range(max_retries): | |
| for attempt in range(self.max_attempts): |
| except Exception as e: | ||
|
|
||
| if "UNAVAILABLE" in str(e) or "DEADLINE_EXCEEDED" in str(e): | ||
| if attempt == max_retries - 1: |
There was a problem hiding this comment.
| if attempt == max_retries - 1: | |
| if attempt == self.max_attempts - 1: |
| except Exception as e: | ||
|
|
||
| if "UNAVAILABLE" in str(e) or "DEADLINE_EXCEEDED" in str(e): | ||
| if attempt == max_retries - 1: |
There was a problem hiding this comment.
we can add backoff if we want to retry similar to what we do in the execute() function of the a normal Query
def backoff(self, attempt: int):
delay = min(0.5 * (2 ** attempt), self._max_backoff)
time.sleep(delay)| def get_transaction(self) -> Optional["Transaction"]: | ||
| return self._transaction | ||
|
|
||
| def execute(self, client) -> FeeEstimateResponse: |
There was a problem hiding this comment.
consider splitting into smaller helper methods to reduce complexity.
|
|
||
| max_retries = getattr(client, "max_retries", 3) | ||
|
|
||
| import requests |
There was a problem hiding this comment.
can be remove duplicate import
|
|
||
| if response.status_code == 400: | ||
| raise ValueError("INVALID_ARGUMENT") | ||
|
|
There was a problem hiding this comment.
can impose retry logic on stautus code like:
408, 429, 5xx
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
WalkthroughThis PR implements HIP-1261 fee estimation support for the Hiero Python SDK. It introduces fee data models, a FeeEstimateMode enum, a FeeEstimateQuery class that integrates with the Mirror Node API endpoint, a convenience method on Transaction, updates to handle string account IDs, and comprehensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SDK as SDK<br/>(Python)
participant MirrorNode as Mirror Node<br/>(/api/v1/network/fees)
User->>SDK: transaction.estimate_fee()<br/>.set_mode(mode).execute(client)
SDK->>SDK: Validate transaction is set
SDK->>SDK: Serialize transaction to protobuf
SDK->>MirrorNode: POST /api/v1/network/fees<br/>?mode=STATE or INTRINSIC<br/>[protobuf binary]
alt Transient Error (UNAVAILABLE/DEADLINE_EXCEEDED)
MirrorNode-->>SDK: Exception
SDK->>SDK: Retry (up to max_retries)
SDK->>MirrorNode: POST /api/v1/network/fees [retry]
else Permanent Error
MirrorNode-->>SDK: HTTP 400 or other error
SDK->>SDK: Raise ValueError
SDK-->>User: Exception
else Success
MirrorNode-->>SDK: 200 OK + JSON response<br/>{node, service, network, mode, notes}
SDK->>SDK: Parse JSON into fee components
SDK->>SDK: Compute network_total = node.subtotal × network.multiplier
SDK->>SDK: Compute total = node + network + service
SDK->>SDK: Construct FeeEstimateResponse
SDK-->>User: FeeEstimateResponse<br/>(mode, node_fee, service_fee,<br/>network_fee, total, notes)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
⚔️ Resolve merge conflicts
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: 10
♻️ Duplicate comments (1)
src/hiero_sdk_python/query/fee_estimate_query.py (1)
48-60:⚠️ Potential issue | 🔴 CriticalUse the actual
ClientREST/retry API here.The concrete client exposes
network.get_mirror_rest_url()andmax_attempts, notmirror_network/max_retries. With a realClient, Line 48 raisesAttributeError, and the retry loop ignores the caller’s configured attempt budget.🐛 Suggested fix
- url = f"{client.mirror_network}/api/v1/network/fees?mode={mode.value}" + url = f"{client.network.get_mirror_rest_url()}/network/fees?mode={mode.value}" @@ - max_retries = getattr(client, "max_retries", 3) + max_retries = client.max_attemptsVerify against the actual client/network definitions:
#!/bin/bash set -euo pipefail rg -n '\bmirror_network\b|\bmax_retries\b|\bmax_attempts\b|get_mirror_rest_url' \ src/hiero_sdk_python/query/fee_estimate_query.py \ src/hiero_sdk_python/client/client.py \ src/hiero_sdk_python/client/network.py
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f0c538d-669e-4bc6-a84f-971b8df7fb8e
📒 Files selected for processing (10)
CHANGELOG.mdsrc/hiero_sdk_python/fees/fee_estimate.pysrc/hiero_sdk_python/fees/fee_estimate_mode.pysrc/hiero_sdk_python/fees/fee_estimate_response.pysrc/hiero_sdk_python/fees/fee_extra.pysrc/hiero_sdk_python/fees/network_fee.pysrc/hiero_sdk_python/query/fee_estimate_query.pysrc/hiero_sdk_python/transaction/transaction.pysrc/hiero_sdk_python/transaction/transfer_transaction.pytests/unit/test_fee_estimate_query.py
| node_fee: FeeEstimate | ||
| service_fee: FeeEstimate | ||
| notes: List[str] = field(default_factory=list) | ||
| total: int = 0 |
There was a problem hiding this comment.
Avoid storing total as a mutable constructor value; derive it from components.
Line 14 defaults total to 0, which can drift from node_fee/network_fee/service_fee and violate the fee formula invariant. Make total a computed property.
Suggested patch
`@dataclass`(frozen=True)
class FeeEstimateResponse:
@@
service_fee: FeeEstimate
notes: List[str] = field(default_factory=list)
- total: int = 0
+
+ `@property`
+ def total(self) -> int:
+ return self.node_fee.subtotal + self.network_fee.subtotal + self.service_fee.subtotal| def set_transaction(self, transaction: "Transaction") -> "FeeEstimateQuery": | ||
|
|
||
| #if hasattr(transaction, "freeze") and not transaction.is_frozen: | ||
| #transaction.freeze() | ||
|
|
||
| #if hasattr(transaction, "freeze") and not getattr(transaction, "is_frozen", False): | ||
| #transaction.freeze() | ||
|
|
||
| self._transaction = transaction | ||
| return self |
There was a problem hiding this comment.
The supplied transaction is never serialized or chunked.
set_transaction() only stores the object, while execute() always iterates over [b"dummy"] and posts b"dummy". That makes every transaction type produce the same estimate and prevents chunked transactions from ever aggregating per-chunk fees. The auto-freeze behavior called out in the PR objective is also effectively absent because the transaction bytes are never derived from self._transaction.
Also applies to: 50-65
| parsed = self._parse_response(data) | ||
|
|
||
| node_total += parsed.node_fee.subtotal | ||
| service_total += parsed.service_fee.subtotal | ||
|
|
||
| network_multiplier = parsed.network_fee.multiplier | ||
| notes.extend(parsed.notes) | ||
|
|
||
| break | ||
| except Exception as e: | ||
|
|
||
| if "UNAVAILABLE" in str(e) or "DEADLINE_EXCEEDED" in str(e): | ||
| if attempt == max_retries - 1: | ||
| raise | ||
| continue | ||
|
|
||
| raise | ||
|
|
||
| network_total = node_total * network_multiplier | ||
| total = node_total + service_total + network_total | ||
|
|
||
| return FeeEstimateResponse( | ||
| mode=mode, | ||
| node_fee=FeeEstimate(base=node_total, extras=[]), | ||
| service_fee=FeeEstimate(base=service_total, extras=[]), | ||
| network_fee=NetworkFee( | ||
| multiplier=network_multiplier, | ||
| subtotal=network_total | ||
| ), | ||
| notes=notes, | ||
| total=total | ||
| ) |
There was a problem hiding this comment.
Preserve the parsed response instead of rebuilding it from request state.
_parse_response() already deserializes the Mirror Node payload, but execute() throws that object away and returns a new FeeEstimateResponse built from aggregated subtotals plus mode=mode from the request. That can misreport what the service actually returned and drops any detail _parse_response() captured. Aggregate the parsed responses instead of reconstructing a new one from request-side values.
Also applies to: 116-140
| def _add_hbar_transfer( | ||
| self, account_id: AccountId, amount: Union[int, Hbar], is_approved: bool = False | ||
| ) -> "TransferTransaction": | ||
| """ | ||
| Internal method to add a HBAR transfer to the transaction. | ||
|
|
||
| Args: | ||
| account_id (AccountId): The account ID of the sender or receiver. | ||
| amount (Union[int, Hbar]): The amount of the HBAR to transfer. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align public type hints/docstrings with the new str account-id behavior.
At Line 63 and Lines 100-116, runtime now accepts str, but method signatures/docs still advertise AccountId only. This creates a public API contract mismatch (and static typing friction) for the newly supported path.
Suggested patch
- def _init_hbar_transfers(self, hbar_transfers: Dict[AccountId, int]) -> None:
+ def _init_hbar_transfers(self, hbar_transfers: Dict[AccountId | str, int]) -> None:
@@
- def _add_hbar_transfer(
- self, account_id: AccountId, amount: Union[int, Hbar], is_approved: bool = False
+ def _add_hbar_transfer(
+ self, account_id: AccountId | str, amount: Union[int, Hbar], is_approved: bool = False
) -> "TransferTransaction":
@@
- account_id (AccountId): The account ID of the sender or receiver.
+ account_id (AccountId | str): The account ID of the sender or receiver.
@@
- def add_hbar_transfer(self, account_id: AccountId, amount: Union[int, Hbar]) -> "TransferTransaction":
+ def add_hbar_transfer(self, account_id: AccountId | str, amount: Union[int, Hbar]) -> "TransferTransaction":
@@
- account_id (AccountId): The account ID of the sender or receiver.
+ account_id (AccountId | str): The account ID of the sender or receiver.
@@
- def add_approved_hbar_transfer(
- self, account_id: AccountId, amount: Union[int, Hbar]
+ def add_approved_hbar_transfer(
+ self, account_id: AccountId | str, amount: Union[int, Hbar]
) -> "TransferTransaction":
@@
- account_id (AccountId): The account ID of the sender or receiver.
+ account_id (AccountId | str): The account ID of the sender or receiver.As per coding guidelines, “Any change to public methods … includes backwards-compatibility analysis.”
Also applies to: 100-116
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 63-63: src/hiero_sdk_python/transaction/transfer_transaction.py#L63
Method _add_hbar_transfer has a cyclomatic complexity of 9 (limit is 8)
| def _add_hbar_transfer( | ||
| self, account_id: AccountId, amount: Union[int, Hbar], is_approved: bool = False | ||
| ) -> "TransferTransaction": | ||
| """ | ||
| Internal method to add a HBAR transfer to the transaction. | ||
|
|
||
| Args: | ||
| account_id (AccountId): The account ID of the sender or receiver. | ||
| amount (Union[int, Hbar]): The amount of the HBAR to transfer. | ||
| is_approved (bool, optional): Whether the transfer is approved. Defaults to False. | ||
|
|
||
| Returns: | ||
| TransferTransaction: The current instance of the transaction for chaining. | ||
| """ | ||
| self._require_not_frozen() | ||
| if isinstance(account_id, str): | ||
| account_id = AccountId.from_string(account_id) | ||
| if not isinstance(account_id, AccountId): | ||
| raise TypeError("account_id must be an AccountId instance.") | ||
| if isinstance(amount, Hbar): | ||
| amount = amount.to_tinybars() | ||
| elif not isinstance(amount, int): | ||
| raise TypeError("amount must be an int or Hbar instance.") | ||
| if amount == 0: | ||
| raise ValueError("Amount must be a non-zero value.") | ||
| if not isinstance(is_approved, bool): | ||
| raise TypeError("is_approved must be a boolean.") | ||
|
|
||
| for transfer in self.hbar_transfers: | ||
| if transfer.account_id == account_id: | ||
| transfer.amount += amount | ||
| return self | ||
|
|
||
| self.hbar_transfers.append( | ||
| HbarTransfer(account_id, amount, is_approved)) | ||
| return self |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reduce _add_hbar_transfer branching to address complexity hotspot.
Line 63 currently combines parsing, validation, normalization, and accumulation. Splitting normalization/validation into helpers will keep this method easier to reason about and maintain (matches the static-analysis warning).
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 63-63: src/hiero_sdk_python/transaction/transfer_transaction.py#L63
Method _add_hbar_transfer has a cyclomatic complexity of 9 (limit is 8)
| def mock_client(): | ||
| client = MagicMock() | ||
| client.mirror_network = "https://testnet.mirrornode.hedera.com" | ||
| client.max_retries = 3 | ||
| return client |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '\bmirror_network\b|\bmax_retries\b|\bmax_attempts\b|get_mirror_rest_url' \
tests/unit/test_fee_estimate_query.py \
src/hiero_sdk_python/client/client.py \
src/hiero_sdk_python/client/network.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 1166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and show FeeEstimateQuery implementation to see what client surface it uses
fd -e py "fee_estimate_query" src/ --type f | head -5Repository: hiero-ledger/hiero-sdk-python
Length of output: 121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what attributes/properties the Client class exposes
cat src/hiero_sdk_python/client/client.py | head -100Repository: hiero-ledger/hiero-sdk-python
Length of output: 3129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what attributes/methods Network class exposes
cat src/hiero_sdk_python/client/network.py | head -100Repository: hiero-ledger/hiero-sdk-python
Length of output: 4344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the FeeEstimateQuery implementation - specifically the execute method
cat src/hiero_sdk_python/query/fee_estimate_query.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 4534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if Client class actually exposes mirror_network attribute anywhere
rg -n "mirror_network" src/hiero_sdk_python/client/client.py
# Check what Network.get_mirror_rest_url() returns
rg -A 5 "def get_mirror_rest_url" src/hiero_sdk_python/client/network.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 55
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's a mirror_network property on Client
rg -B 2 -A 5 "@property" src/hiero_sdk_python/client/client.py | grep -A 5 "mirror"
# Check what attributes Network has
rg -n "def " src/hiero_sdk_python/client/network.py | head -20
# Check the full get_mirror_rest_url method
rg -B 2 -A 10 "def get_mirror_rest_url" src/hiero_sdk_python/client/network.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 55
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the specific lines from network.py where get_mirror_rest_url is defined
sed -n '324,335p' src/hiero_sdk_python/client/network.py
# Check if Client has a reference to network.get_mirror_rest_url or mirror_network
grep -n "get_mirror_rest_url\|mirror_network" src/hiero_sdk_python/client/client.py
# Check the full network.py around line 324
sed -n '320,330p' src/hiero_sdk_python/client/network.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 565
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the actual FeeEstimateQuery code uses client.mirror_network
grep -n "mirror_network" src/hiero_sdk_python/query/fee_estimate_query.py
# Check if Client class exposes mirror_network as property or attribute
grep -n "mirror_network" src/hiero_sdk_python/client/client.py
# Confirm max_attempts is the real attribute name, not max_retries
grep -n "max_attempts\|max_retries" src/hiero_sdk_python/client/client.py | head -10Repository: hiero-ledger/hiero-sdk-python
Length of output: 154
Fix the mock to use the actual Client API, and align FeeEstimateQuery.execute() to the real SDK surface.
The mock invents mirror_network and max_retries attributes, but the real Client class exposes max_attempts (not max_retries) and does not have a mirror_network property. The actual Client.network is a Network instance with a get_mirror_rest_url() method.
More critically, FeeEstimateQuery.execute() itself uses client.mirror_network (line 48), which does not exist on the real Client. This test will not catch the production failure when the code runs against a real client.
Update the mock to use Client's real surface (client.network, client.max_attempts), and refactor FeeEstimateQuery.execute() to call client.network.get_mirror_rest_url() and reference client.max_attempts instead of the invented attributes.
| @patch("hiero_sdk_python.query.fee_estimate_query.requests.post") | ||
| def test_transfer_transaction_intrinsic_mode(mock_post): | ||
|
|
||
| mock_post.return_value = mock_requests_response() | ||
|
|
||
| tx = TransferTransaction() | ||
| tx.add_hbar_transfer("0.0.1001", Hbar(-1)) | ||
| tx.add_hbar_transfer("0.0.1002", Hbar(1)) | ||
|
|
||
| query = ( | ||
| FeeEstimateQuery() | ||
| .set_transaction(tx) | ||
| .set_mode(FeeEstimateMode.INTRINSIC) | ||
| ) | ||
|
|
||
| result = query.execute(mock_client()) | ||
|
|
||
| assert result.mode == FeeEstimateMode.INTRINSIC | ||
|
|
||
| @patch("hiero_sdk_python.query.fee_estimate_query.requests.post") | ||
| def test_default_mode_is_state(mock_post): | ||
|
|
||
| mock_post.return_value = mock_requests_response() | ||
|
|
||
| tx = TransferTransaction() | ||
|
|
||
| query = FeeEstimateQuery().set_transaction(tx) | ||
|
|
||
| result = query.execute(mock_client()) | ||
|
|
||
| assert result.mode == FeeEstimateMode.STATE |
There was a problem hiding this comment.
Assert the outgoing mode parameter, not just the returned enum.
These tests can still pass if the POST URL never includes mode= or always uses the default, because they only inspect result.mode. Check mock_post.call_args so the suite actually protects request construction.
🧪 Suggested assertions
result = query.execute(mock_client())
assert result.mode == FeeEstimateMode.INTRINSIC
+ assert (
+ f"mode={FeeEstimateMode.INTRINSIC.value}" in mock_post.call_args[0][0]
+ ), "FeeEstimateQuery must send the selected mode in the Mirror Node URL"
@@
result = query.execute(mock_client())
assert result.mode == FeeEstimateMode.STATE
+ assert (
+ f"mode={FeeEstimateMode.STATE.value}" in mock_post.call_args[0][0]
+ ), "FeeEstimateQuery must default the Mirror Node URL to STATE mode"As per coding guidelines, "Assert backward-compatible defaults are maintained."
| @patch("hiero_sdk_python.query.fee_estimate_query.requests.post") | ||
| def test_invalid_argument_error(mock_post): | ||
|
|
||
| response = MagicMock() | ||
| response.status_code = 400 | ||
|
|
||
| mock_post.return_value = response | ||
|
|
||
| tx = TransferTransaction() | ||
|
|
||
| query = FeeEstimateQuery().set_transaction(tx) | ||
|
|
||
| with pytest.raises(ValueError): | ||
| query.execute(mock_client()) | ||
|
|
||
| @patch("hiero_sdk_python.query.fee_estimate_query.requests.post") | ||
| def test_retry_on_unavailable(mock_post): | ||
|
|
||
| mock_post.side_effect = [ | ||
| Exception("UNAVAILABLE"), | ||
| mock_requests_response(), | ||
| ] | ||
|
|
||
| tx = TransferTransaction() | ||
|
|
||
| result = FeeEstimateQuery().set_transaction(tx).execute(mock_client()) | ||
|
|
||
| assert result is not None | ||
|
|
||
| @patch("hiero_sdk_python.query.fee_estimate_query.requests.post") | ||
| def test_retry_on_timeout(mock_post): | ||
|
|
||
| mock_post.side_effect = [ | ||
| Exception("DEADLINE_EXCEEDED"), | ||
| mock_requests_response(), | ||
| ] | ||
|
|
||
| tx = TransferTransaction() | ||
|
|
||
| result = FeeEstimateQuery().set_transaction(tx).execute(mock_client()) | ||
|
|
||
| assert result is not None |
There was a problem hiding this comment.
Retry-path tests need to assert retry boundaries.
Right now these cases only prove “eventually failed/succeeded”. A regression that retries INVALID_ARGUMENT or stops retrying transient failures would still pass. Assert call counts and the expected error text.
🧪 Suggested assertions
- with pytest.raises(ValueError):
+ with pytest.raises(ValueError, match="INVALID_ARGUMENT"):
query.execute(mock_client())
+ assert mock_post.call_count == 1, "INVALID_ARGUMENT must not be retried"
@@
result = FeeEstimateQuery().set_transaction(tx).execute(mock_client())
assert result is not None
+ assert mock_post.call_count == 2, "UNAVAILABLE should be retried once before succeeding"
@@
result = FeeEstimateQuery().set_transaction(tx).execute(mock_client())
assert result is not None
+ assert mock_post.call_count == 2, "DEADLINE_EXCEEDED should be retried once before succeeding"As per coding guidelines, "Unit tests should be extensive - test even if we don't expect users to use it currently."
| @patch("hiero_sdk_python.query.fee_estimate_query.requests.post") | ||
| def test_topic_message_single_chunk(mock_post): | ||
|
|
||
| mock_post.return_value = mock_requests_response() | ||
|
|
||
| tx = TopicMessageSubmitTransaction() | ||
| tx.set_message("hello") | ||
|
|
||
| result = FeeEstimateQuery().set_transaction(tx).execute(mock_client()) | ||
|
|
||
| assert result is not None | ||
|
|
||
| @patch("hiero_sdk_python.query.fee_estimate_query.requests.post") | ||
| def test_topic_message_multiple_chunks(mock_post): | ||
|
|
||
| mock_post.return_value = mock_requests_response() | ||
|
|
||
| tx = TopicMessageSubmitTransaction() | ||
| tx.set_message("A" * 5000) | ||
|
|
||
| result = FeeEstimateQuery().set_transaction(tx).execute(mock_client()) | ||
|
|
||
| assert result is not None No newline at end of file |
There was a problem hiding this comment.
The chunking tests never verify chunking.
Both topic-message tests only assert non-None, so a placeholder implementation that always POSTs once still passes. The large-message case should assert multiple requests and aggregated totals across chunk responses.
🧪 Suggested assertions
result = FeeEstimateQuery().set_transaction(tx).execute(mock_client())
assert result is not None
+ assert mock_post.call_count == 1, "Small topic messages should be estimated in a single request"
@@
result = FeeEstimateQuery().set_transaction(tx).execute(mock_client())
assert result is not None
+ assert mock_post.call_count > 1, "Large topic messages should be estimated per chunk"
+ single_total = 10 + 20 + (10 * 2)
+ assert (
+ result.total == single_total * mock_post.call_count
+ ), "Chunked fee estimates must aggregate all chunk responses"As per coding guidelines, "Unit tests should be extensive - test even if we don't expect users to use it currently."
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
|
Hello, this is the OfficeHourBot. This is a reminder that the Hiero Python SDK Office Hours are scheduled in approximately 4 hours (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, |
3 participants
Description:
This PR aims to add in the implementation for HIP-1261.
Related issue(s):
Fixes #1633
Notes for reviewer:
Checklist