Follow-up from "Draft: msobczyk/cli update witness"

The following discussions from !781 (merged) should be addressed:

@mzebrak started a discussion: (+4 comments)

should rather be name or witness-name
@mzebrak started a discussion: (+9 comments)

Looks like we might consider having a parser for this account-subsidy-decay and account-subsidy-budget and maximum-block-size. Looks like can consts can be placed in __private/core/constants/node.py
@mzebrak started a discussion: (+2 comments)

THis is not strictly part of this MR but I just noticed that.

Looks like these constants UNKNOWN_ACCOUNT, KNOWN_ACCOUNTS, KNOWN_EXCHANGE_NAMES doest not fit in this module because in this module there are constants stored that are related with testnet blochain /block log configuration. I think these should be moved to /clive-local-tools/clive_local_tools/data/constants.py. I'd also add _NAME postfix
@mzebrak started a discussion: (+1 comment)

I think clive should allow for passing public key as str | PublicKey in WitnessSetProperties wrapper
@mzebrak started a discussion: (+2 comments)

please add simple 1-2 line docstrings explaining the purpose of each test

only a single operation, so do not store in a list

    expected_operation_type: type[OperationBase] = (
        WitnessUpdateOperation if use_active_authority else WitnessSetPropertiesOperation
    )

@mzebrak started a discussion:

to avoid such a long names I suggest to create an alias in the ARRANGE section like cli_tester = cli_tester_unlocked_with_witness_profile
@mzebrak started a discussion: (+2 comments)

this is basically witness_name and should be placed in the ARRANGE section
@mzebrak started a discussion:

No, we shouldn't check the blockchain behaviour but just verify that the correct operation was sent to blockchain. Instead of assert_operation_type_in_blockchain and manually checking if blockchain changed the witness, use assert_operations_placed_in_blockchain to check for exact operation and remove commands.find_witness call

@mzebrak started a discussion:

thats basically

    expected_account_creation_fee = tt.Asset.Hive(3.456)

@mzebrak started a discussion: (+2 comments)
These tests are very similar and maybe can be placed in a parametrized test, but let see after other changes are done:
- test_account_creation_fee
- test_maximum_block_size
- test_hbd_interest_rate
- test_new_signing_key
- test_hbd_exchange_rate
- test_url
- test_account_subsidy_budget (but doesn't run use_active_authority)
- test_account_subsidy_decay (but doesn't run use_active_authority)
@mzebrak started a discussion: (+4 comments)
The main issue with these tests is that they don't check if --use-active-authorty flag works because this witness has the same ACTIVE and WITNESS key. I think active key should be different from witness key. Let's store witness key separately. I think we should utilize similar thing to AccountData, but name it WitnessData and might inherit from AccountData

see:
- https://gitlab.syncad.com/hive/clive/-/blob/9c640286a663279cbfe0813a2cb537fadcc35ff6/tests/clive-local-tools/clive_local_tools/testnet_block_log/generate_block_log.py#L74
- https://gitlab.syncad.com/hive/clive/-/blob/ae98ba002cc4d495c8266ac510c24010ae3b3f6e/tests/functional/cli/conftest.py#L211
@mzebrak started a discussion: (+1 comment)

Looks like this help text can be rearranged and maybe reuse the default param of stylized_help introduced in !783. Linked thread: !783 (comment 231764)

@mzebrak started a discussion: (+4 comments)

This check was removed and now its possible to create empty transaction. Looks like this should be a dedicated validation method that will be done in

    @override
    async def validate(self) -> None:

currently:

(venv) haf_admin@b6391f0e3dad:/workspace/clive$ clive process update-witness --sign-with witness-000_key
Using beekeeper at http://127.0.0.1:38553
╭─ Error ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ Communication error with http://127.0.0.1:8090:                                                                           │
│ Assert Exception:operations.size() > 0: A transaction must have at least one operation                                    │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

@mzebrak started a discussion:

all the CLI errors start with CLI prefix
class CLIRequiresWitnessSetPropertiesOperationError(CLIPrettyError):

@mzebrak started a discussion: (+4 comments)
Not sure if we should deal with the proto, maybe a better solution would be to pass wax.WitnessSetProperties into wax.Transaction and then tx.to_json() then load this transaction into schemas/clive transaction and retrieve the first operation from it? This way won't need to implement _props_to_schemas. looks like there wax_interface.create_transaction methods available`. Related !781 (comment 232115)

And I think we should assert
```
        proto_operations = self._operation.finalize(wax_interface)
        assert len(proto_operations) == 1, "A single proto operation was expected from wax WitnessSetProperties"
        proto_operation = proto_operations[0]
```
@mzebrak started a discussion:

Looks like this find_witness call doesnt have to be made always because witnesssetproperties doesn't require that. However this thread is related with the exception changes already taking place
@mzebrak started a discussion:

Why is ComposeTransaction used as a return type hint in most places, but not in some? Like there in process_account_update and in process_account_creation
@mzebrak started a discussion:

missing @override

@mzebrak started a discussion:

maybe like that?
    properties = [
        self.account_creation_fee,
        self.maximum_block_size,
        self.hbd_interest_rate,
        self.new_signing_key,
        self.url,
        self.hbd_exchange_rate,
        self.account_subsidy_budget,
        self.account_subsidy_decay,
    ]
    is_witness_set_properties_option_given = any(prop is not None for prop in properties)
    return self.use_witness_key and is_witness_set_properties_option_given
the is not None check can also be abstracted out to smth like def _is_option_given(self, value: object | None)-> bool: I don't like the fact we are tied in so many places to the default None meaning "not set", if we ever want/need to introduce a sentinel object (like if None will become also a valid argument.).

Edited Jan 09, 2026 by Mateusz Żebrak