feat: Add Hip1261 Implementation#2231
Conversation
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
github-actions
Bot
added
notes: HIP
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 73 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #2231 +/- ##
==========================================
+ Coverage 93.72% 93.76% +0.04%
==========================================
Files 145 151 +6
Lines 9507 9714 +207
==========================================
+ Hits 8910 9108 +198
- Misses 597 606 +9 🚀 New features to boost your workflow:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds HIP-1261 fee-estimation support: immutable fee data models and enum, a Mirror Node-backed ChangesFee Estimation Query Implementation
Sequence DiagramsequenceDiagram
actor User
participant FeeEstimateQuery
participant TxnMgmt as TransactionState
participant Client as RESTClient
participant MirrorNode as MirrorNode
User->>FeeEstimateQuery: set_transaction(tx)
FeeEstimateQuery->>TxnMgmt: freeze() if not frozen
FeeEstimateQuery-->>User: self
User->>FeeEstimateQuery: set_mode(STATE/INTRINSIC)
FeeEstimateQuery-->>User: self
User->>FeeEstimateQuery: execute(client)
FeeEstimateQuery->>TxnMgmt: validate and serialize transaction
FeeEstimateQuery->>Client: POST /api/v1/network/fees?mode=... (protobuf)
Note over Client,MirrorNode: Retry on timeout/408/429/5xx with exponential backoff
Client->>MirrorNode: HTTP POST
MirrorNode-->>Client: JSON response (node, service, network)
Client-->>FeeEstimateQuery: JSON
FeeEstimateQuery->>FeeEstimateQuery: _to_response() → FeeEstimateResponse
FeeEstimateQuery-->>User: FeeEstimateResponse
sequenceDiagram
actor User
participant FeeEstimateQuery
participant TxnState as TransactionState
participant Client as RESTClient
participant Aggregator as FeeAggregator
User->>FeeEstimateQuery: execute(client) with chunked Tx
FeeEstimateQuery->>FeeEstimateQuery: _is_chunked() -> true
loop for each chunk id
FeeEstimateQuery->>TxnState: set transaction_id to chunk id
FeeEstimateQuery->>TxnState: clear cached body bytes
FeeEstimateQuery->>TxnState: freeze chunk
FeeEstimateQuery->>Client: POST chunk protobuf
Client-->>FeeEstimateQuery: JSON chunk fees
FeeEstimateQuery->>Aggregator: accumulate node/service/base/extras & network subtotal
end
FeeEstimateQuery->>TxnState: restore original transaction state
Aggregator-->>FeeEstimateQuery: aggregated FeeEstimateResponse
FeeEstimateQuery-->>User: aggregated FeeEstimateResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 (2)
tests/integration/fee_estimate_query_e2e_test.py (1)
49-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix typo and improve test structure.
Same typo issue (
chuck_tx→chunk_tx). Removeprint()and add meaningful assertions for chunked transaction fee aggregation.Proposed fix
`@pytest.mark.integration` -def test_can_execute_fee_estimation_query_chuck_tx(env): - +def test_fee_estimation_query_topic_message_chunked(env): tx = ( TopicMessageSubmitTransaction().set_topic_id(TopicId(0, 0, 2)).set_chunk_size(10).set_message("s" * 20) ) # 2 chunks - # 2. IMPORTANT: Let freeze_with generate the valid transaction ID sequence - # This ensures tx._transaction_ids is populated correctly. - tx.freeze_with(env.client) query = FeeEstimateQuery().set_mode(FeeEstimateMode.STATE).set_transaction(tx) result = query.execute(env.client) - print(result) assert result is not None + assert result.total > 0, "Chunked transaction should have positive fee total" + assert result.node_fee is not None, "Should include node fee breakdown" + assert result.service_fee is not None, "Should include service fee breakdown"src/hiero_sdk_python/query/fee_estimate_query.py (1)
300-309:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSame
isinstance()syntax error with union types.This line has the same issue as line 102 - union types with
|are not valid inisinstance().Proposed fix
def _is_chunked(self) -> bool: from hiero_sdk_python.consensus.topic_message_submit_transaction import ( TopicMessageSubmitTransaction, ) from hiero_sdk_python.file.file_append_transaction import ( FileAppendTransaction, ) - return isinstance(self._transaction, (TopicMessageSubmitTransaction | FileAppendTransaction)) + return isinstance(self._transaction, (TopicMessageSubmitTransaction, FileAppendTransaction))
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f98fb1e-c6c0-4127-8eb5-5dd9450e94a1
📒 Files selected for processing (8)
src/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.pytests/integration/fee_estimate_query_e2e_test.pytests/unit/fee_estimate_query_test.py
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f551bba7-22c5-4d85-b359-337737374df2
📒 Files selected for processing (3)
src/hiero_sdk_python/fees/fee_estimate.pysrc/hiero_sdk_python/query/fee_estimate_query.pytests/integration/fee_estimate_query_e2e_test.py
|
Hi, this is WorkflowBot.
|
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 664f5aca-38f0-4b43-98c0-0b4ea28ad32d
📒 Files selected for processing (1)
src/hiero_sdk_python/query/fee_estimate_query.py
|
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, |
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hiero_sdk_python/consensus/topic_message_submit_transaction.py (2)
204-209:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBLOCKER: Guard
initialTransactionIDbefore multi-chunk serialization.When
_total_chunks > 1,self._initial_transaction_id._to_proto()can dereferenceNoneif chunk metadata wasn’t initialized first. Add an explicit guard and raise a descriptiveValueError.Proposed fix
# Multi-chunk metadata if self._total_chunks > 1: + if self._initial_transaction_id is None: + raise ValueError( + "initial transaction ID is missing for multi-chunk submit; call freeze_with(client) first." + ) body.chunkInfo.CopyFrom( consensus_submit_message_pb2.ConsensusMessageChunkInfo( initialTransactionID=self._initial_transaction_id._to_proto(), total=self._total_chunks, number=self._current_chunk_index + 1, ) )As per coding guidelines, "
_build_proto_body()callsself._initial_transaction_id._to_proto()when_total_chunks > 1... an explicit guard with a descriptiveValueErrorMUST be present."
269-271:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBLOCKER:
Timestamp.nanosoverflow is not handled when generating chunk transaction IDs.
nanos=base_timestamp.nanos + ican exceed protobuf bounds (999_999_999) for large chunk counts. Carry overflow intoseconds.Proposed fix
else: + total_nanos = base_timestamp.nanos + i chunk_valid_start = timestamp_pb2.Timestamp( - seconds=base_timestamp.seconds, nanos=base_timestamp.nanos + i + seconds=base_timestamp.seconds + total_nanos // 1_000_000_000, + nanos=total_nanos % 1_000_000_000, ) chunk_transaction_id = TransactionId( account_id=self.transaction_id.account_id, valid_start=chunk_valid_start )As per coding guidelines, "the expression
base_timestamp.nanos + iMUST be checked for overflow ... An overflow guard MUST carry seconds over."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 880bc1b9-1aaf-4304-ad91-ff405c830163
📒 Files selected for processing (4)
src/hiero_sdk_python/consensus/topic_message_submit_transaction.pysrc/hiero_sdk_python/query/fee_estimate_query.pysrc/hiero_sdk_python/transaction/transaction.pytests/unit/fee_estimate_query_test.py
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/hiero_sdk_python/query/fee_estimate_query.py (1)
31-52:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
FeeEstimateQuerybypasses the SDKQuerycontract by not extendingQuery.This class currently implements a parallel execution stack (custom execute/retry flow) instead of participating in base
Querybehavior. That risks divergence in query semantics and lifecycle guarantees.As per coding guidelines, "All queries MUST: Use the base
Queryexecution flow" and "Subclasses MUST NOT: Override retry logic."Also applies to: 114-187
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c270fd80-3944-4747-a57b-4dc8fd3308e1
📒 Files selected for processing (2)
src/hiero_sdk_python/query/fee_estimate_query.pytests/unit/fee_estimate_query_test.py
github-actions
Bot
added
the
queue:junior-committer
manishdait
left a comment
manishdait
left a comment
There was a problem hiding this comment.
Hi @aceppaluni, nice work! Just a few nitpicks, and this should be ready to go.
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
github-actions
Bot
added
queue:maintainers
Akshat8510
left a comment
Akshat8510
left a comment
There was a problem hiding this comment.
LGTM 👍 Nice work on this implementation! 🚀
github-actions
Bot
added
status: ready-to-merge
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
github-actions
Bot
added
queue:junior-committer
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
github-actions
Bot
added
queue:maintainers
github-actions
Bot
added
status: ready-to-merge
Signed-off-by: aceppaluni <aceppaluni@gmail.com> Signed-off-by: darshit2308 <darshit2308@gmail.com>
Signed-off-by: aceppaluni <aceppaluni@gmail.com> Signed-off-by: darshit2308 <darshit2308@gmail.com>
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
4 participants
Description:
This PR aims to add in the implementation for HIP-1261.
Related issue(s):
Fixes #1633
Notes for reviewer:
Old PR was #2019
Checklist