hiero-ledger / hiero-ledger/hiero-sdk-cpp

[Intermediate]: Apply `applyIfSet` helper consistently across TCK service update methods

Open
#1,612 0 comments 0 reactions 0 assignees View on GitHub
priority: low scope: tests skill: intermediate status: ready for dev
Dominant language
C++
Stars
42
Forks
108
Avg merge
11h 45m
Merged PRs (30d)
2

Description

### 🧩 Intermediate Friendly

This issue is a good fit for contributors who are already familiar with the Hiero C++ SDK and feel comfortable navigating the codebase.

Intermediate Issues often involve:
- Exploring existing implementations
- Understanding how different components work together
- Making thoughtful changes that follow established patterns

The goal is to support deeper problem-solving while keeping the task clear, focused, and enjoyable to work on.

> [!IMPORTANT]
> ### 🧭 About Intermediate Issues
>
> Intermediate Issues are a great next step for contributors who enjoy digging into the codebase and reasoning about how things work.
>
> These issues often:
> - Involve multiple related files or components
> - Encourage investigation and understanding of existing behavior
> - Leave room for thoughtful implementation choices
> - Stay focused on a clearly defined goal
>
> Other kinds of contributions — from beginner-friendly tasks to large system-level changes — are just as valuable and use different labels.

### 👾 Description of the Task

PR #1424 (TCK `updateContract`) introduced an `applyIfSet`-style abstraction for conditionally calling setters on a transaction when a request field has a value. Reviewer feedback was that if the abstraction is worth adopting, it should be applied consistently across _all_ TCK service update methods rather than only `updateContract`:

> Maintainer comment on PR #1424: *"if the `applyIfSet` abstraction is worth adopting, it should be applied consistently across all TCK service update methods — not just `updateContract`. Please open a follow-up issue for that…"*
> Author reply: *"Thanks for clarifying I will create a follow up issue."*

The follow-up was never opened. Verified at the v0.55.0 release commit: `applyIfSet` / `applyOptionalValue` appear nowhere under `src/tck/`. So the original `updateContract` may now also be inconsistent with itself — confirm during the work below and decide whether to introduce or remove the helper.

Relevant directories:

```
src/tck/src/ — every *Service.cc that exposes an update method is in scope
src/tck/include/
src/tck/tests/ — TCK unit tests
```

Concretely, the TCK update surfaces likely affected (the exact list should be re-derived during the work):

```
updateAccount (AccountService)
updateContract (ContractService — the one PR #1424 originally touched)
updateFile (FileService)
updateToken (TokenService)
updateTopic (ConsensusService)
updateNode (NetworkService / NodeService, if exposed)
updateSchedule (ScheduleService — if applicable; see PR #1592 for the latest sign/getInfo work)
```

### 💡 Proposed Approach

Two valid directions — pick one, justify in the PR description:

1. **Adopt the helper everywhere.** Promote `applyIfSet` (or whatever it is currently called inside the `ContractService`) to a shared TCK utility (e.g. `src/tck/include/util/ApplyIfSet.h`) and use it across every TCK update method. The code in each service should look uniform.

2. **Drop the helper.** If, on closer reading, the `if (req.field.has_value()) tx.setField(*req.field);` shape is _already_ the established pattern across other services and the helper only complicates `updateContract`, remove it from `updateContract` and align that method with the rest.

Direction (1) is the maintainer's stated preference but only if the abstraction earns its keep. If you find the helper saves more than 3–5 lines per service _and_ removes a real footgun (e.g. forgetting to dereference an optional), keep it. Otherwise (2) is the right call.

Either way, after this issue lands, every TCK update method should look the same.

### 👩‍💻 Implementation Steps

- [ ] Read `src/tck/src/contract/ContractService.cc` (the file PR #1424 touched) and locate the `applyIfSet`-style abstraction. Confirm where it lives today and what it does.
- [ ] Inventory every TCK update method by grepping for `update`-suffixed handlers under `src/tck/src/`. Make a written list.
- [ ] For each service, note the current style (helper-using, hand-rolled `if has_value()`, or some mix).
- [ ] Decide: adopt the helper everywhere, or remove it from `updateContract`. Document the decision in the PR description with one or two concrete code-shape examples comparing before/after.
- [ ] Apply the chosen direction across all TCK update methods. Keep behavior identical — this is purely a uniformity refactor.
- [ ] If adopting the helper, hoist it into a shared header (`src/tck/include/util/ApplyIfSet.h` or similar) and add a small unit test exercising the optional/required branches.
- [ ] If removing the helper, delete the helper from `ContractService.cc` and make sure no other files referenced it.
- [ ] Run the TCK test suite locally (`-DBUILD_TCK_TESTS=ON`) and confirm everything passes.

### ✅ Acceptance Criteria

- [ ] Every TCK update method (`updateAccount`, `updateContract`, `updateFile`, `updateToken`, `updateTopic`, `updateNode`, `updateSchedule`, etc.) follows the same pattern for conditionally applying optional setters.
- [ ] The PR description states which direction was chosen (adopt the helper, or remove it) and gives a brief rationale.
- [ ] If the helper is kept, it lives in a shared TCK util header and has at least one unit test.
- [ ] No behavior change in any TCK request/response handling — same inputs produce the same outputs.
- [ ] The TCK test suite passes.

---

### 📋 Step-by-Step Contribution Guide

To help keep contributions consistent and easy to review, we recommend following these steps:

- [ ] Comment `/assign` to request the issue
- [ ] Wait for assignment
- [ ] Fork the repository and create a branch
- [ ] Set up the project using the instructions in `README.md`
- [ ] Make the requested changes
- [ ] Sign each commit using `-s -S`
- [ ] Push your branch and open a pull request

Read [Workflow Guide](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/docs/training/workflow.md) for step-by-step workflow guidance.
Read [README.md](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/README.md) for setup instructions.

❗ Pull requests **cannot be merged** without `S` and `s` signed commits.
See the [Signing Guide](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/docs/training/signing.md).

### 🤔 Additional Information

- Originating PR: [#1424](https://github.com/hiero-ledger/hiero-sdk-cpp/pull/1424).
- Closing issue from that PR: [#1377](https://github.com/hiero-ledger/hiero-sdk-cpp/issues/1377) (closed) — context only; no acceptance criteria there cover this follow-up.
- Build with `-DBUILD_TCK=ON -DBUILD_TCK_TESTS=ON` to ensure all TCK code paths compile.

If you have questions while working on this issue, feel free to ask! [Hiero-SDK-C++ Discord](https://discord.com/channels/905194001349627914/1337424839761465364)

Contributor guide

Open the contributing guide

Research direction

Read src/tck/src/contract/ContractService.cc and inventory update handlers by searching under src/tck/src/. Compare each service's optional-setter pattern, including AccountService, FileService, TokenService, ConsensusService, NetworkService or NodeService, and ScheduleService where present. Run the TCK tests with BUILD_TCK_TESTS=ON; done means every in-scope update method follows one justified pattern without behavior changes and the suite passes.

Written by the indexing model from the issue text.

Assessment

Tech stack
cmake, cpp
Domain
backend-api-design, testing-qa
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.