feat(tck): implement updateTopic JSON-RPC method#2462
Conversation
Signed-off-by: achintya2k5 <achintyasin@gmail.com>
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds the ChangesTopic update operation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant JSONRPC
participant update_topic
participant TopicUpdateTransaction
participant HederaNetwork
JSONRPC->>update_topic: UpdateTopicParams
update_topic->>TopicUpdateTransaction: Build and set provided fields
update_topic->>HederaNetwork: Execute transaction
HederaNetwork-->>update_topic: Receipt status
update_topic-->>JSONRPC: UpdateTopicResponse
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
📋 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.
Pull request overview
Adds the missing updateTopic JSON-RPC method to the TCK server so TopicUpdateTransaction test suites can run, aligning the implementation with existing topic/account handler patterns.
Changes:
- Introduces
UpdateTopicParamsrequest model (JSON-RPC parsing/validation) andUpdateTopicResponsepayload. - Implements
_build_update_topic_transaction()plusupdate_topic()handler registered asupdateTopic. - Adds unit tests covering param parsing, transaction building, and handler registry wiring.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/tck/topic_test.py | Adds new unit tests for updateTopic param parsing, transaction building, and registry registration. |
| tck/response/topic.py | Adds UpdateTopicResponse dataclass for updateTopic responses. |
| tck/param/topic.py | Adds UpdateTopicParams dataclass and JSON-RPC parsing for updateTopic. |
| tck/handlers/topic.py | Adds TopicUpdateTransaction builder and updateTopic JSON-RPC handler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert transaction.topic_id == TopicId.from_string("0.0.100") | ||
| assert transaction.memo == "" | ||
| assert transaction.admin_key is None | ||
| assert transaction.submit_key is None | ||
| assert transaction.auto_renew_period == Duration(7890000) | ||
| assert transaction.auto_renew_account is None | ||
| assert transaction.expiration_time is None |
| @dataclass | ||
| class UpdateTopicResponse: | ||
| """Response payload for updateTopic""" | ||
|
|
||
| status: str | None = None | ||
|
|
github-actions
Bot
added
open to community review
aceppaluni
added
status: update branch
github-actions
Bot
added
open to community review
|
Thank you for the review! I'll make the suggested changes and update this PR soon |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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: 94aea267-1092-492d-8192-ffdea6a0306e
📒 Files selected for processing (3)
tck/handlers/topic.pytck/param/topic.pytck/response/topic.py
Signed-off-by: achintya2k5 <achintyasin@gmail.com>
8a34b87 to
8c8d0ae
Compare
There was a problem hiding this comment.
@manishdait I think we have an issue here with the SDK
if set_auto_renew_period is str, we have Duration | int in the SDK
So we could either
temporarily override the str to int, so the tests pass, and give us time to correct the SDK in a different PR
e.g
transaction.set_auto_renew_period(to_int(params.autoRenewPeriod))
transaction.set_expiration_time(Timestamp(seconds=to_int(params.expirationTime), nanos=0))
or we correct the SDK as the tck tests will fail if set correct
auto_renew_period=Duration(7890000) default --> an update transaction must mean "no change" for any field the caller didn't set ---> JS SDK defaults to unset
a TopicUpdateTransaction that never touches memo still sends StringValue("") — which clears the topic's memo on-chain
_build_proto_body raising ValueError("Missing required fields: topic_id") — defer business validation to the network
Since the issue is more complicated than anticipated, i'm also happy to make the additional changes to the PR and do it as a joint effort
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
We need to change class TopicUpdateTransaction(Transaction) regarding auto_renew_period, memo, set_expiration_time, _build_proto_body to ensure consistency with what the tck standards expect
|
@achintya2k5 and @manishdait what is your preference? |
exploreriii
removed
status: update branch
|
I think using transaction.set_auto_renew_period(to_int(params.autoRenewPeriod))
transaction.set_expiration_time(Timestamp(seconds=to_int(params.expirationTime), nanos=0))is fine for the tck handlers. The Also we should still fix So I am +1 on handling the conversion in the tck for now, while fixing the sdk behavior in a separate PR. |
|
I agree with @manishdait , handling the conversion in the TCK handler for this PR. The handler is responsible for adapting the JSON-RPC request into the SDK types, so converting I also feel that the If you'd like, I'd also be happy to contribute to the SDK changes in that separate issue. If everyone agrees with this approach, I'll update this PR with the handler-side conversions that @manishdait suggested so we can get this one ready to merge. |
|
Yes please, when this issue is completed, please do help by opening the other one :) |
Description:
Add the
updateTopicJSON-RPC method to the TCK server, following the existingcreateTopic/updateAccounthandler conventions.UpdateTopicParamsdataclass intck/param/topic.pywithtopicId,memo,adminKey,submitKey,autoRenewPeriod,autoRenewAccountId,expirationTime,feeScheduleKey,feeExemptKeys, andcustomFeesfields, plus JSON-RPC param parsing and validationUpdateTopicResponsedataclass intck/response/topic.py_build_update_topic_transaction()helper andupdate_topic()handler intck/handlers/topic.py, registered via@rpc_method("updateTopic")tests/tck/topic_test.pyfor param parsing, transaction building (including set-only-present-fields behavior fortopicIdandexpirationTime), and registry wiringRelated issue(s):
Fixes #2426
Notes for reviewer:
All 11 new tests pass (
pytest tests/tck/topic_test.py -v). Implementation mirrors the existingcreateTopicandupdateAccounthandlers for consistency.Checklist