Idempotent proposals

[letmaik]

#1791

old name	new name	idempotency
set_js_app	(remove)	ok
deploy_js_app	(unchanged)	ok
remove_js_app	(unchanged)	ok
set_module	(unchanged)	ok
remove_module	(unchanged)	fails if module missing
new_member	add_member	ok
retire_member	(unchanged)	fails if member missing or not active
set_member_data	(unchanged)	ok
new_user	add_user	fails (EXCEPTION!) if user exists
remove_user	(unchanged)	fails if user missing
set_user_data	(unchanged)	ok
set_ca_cert	(unchanged)	ok
remove_ca_cert	(unchanged)	ok
set_jwt_issuer	(unchanged)	ok
remove_jwt_issuer	(unchanged)	fails if issuer missing
set_jwt_public_signing_keys	(unchanged)	ok
trust_node	(unchanged)	ok
retire_node	(unchanged)	fails if node already retired
new_node_code	trust_node_code (?)	fails if code id already exists
retire_node_code	(unchanged)	fails if code id already retired (removed)
accept_recovery	(unchanged)	?
open_network	(unchanged)	fails if network already open
rekey_ledger	(unchanged)	ok
update_recovery_shares	refresh_recovery_shares (?)	ok
set_recovery_threshold	(unchanged)	ok

I'm not too sure about the verbs, it seems like "set" and "remove" doesn't fit everywhere, e.g. when adding a new user, or trusting a node. Any opinions? I'll update the table once we agree on things.

[achamayou]

I'm also unsure about verbs, I'm wondering about something like $NOUN_present, $NOUN_absent? I would like them to be statements of fact, rather than actions.

old name	new name	idempotency
deploy_js_app	js_app_present	ok
remove_js_app	js_app_absent	ok
set_module	module_present	ok
remove_module	module_absent	fails if module missing
new_member	member_present	ok
retire_member	member_absent	fails if member missing or not active
set_member_data	member_data_present	ok
new_user	user_present	fails (EXCEPTION!) if user exists
remove_user	user_absent	fails if user missing
set_user_data	user_data_present	ok
set_ca_cert	ca_cert_present	ok
remove_ca_cert	ca_cert_absent	ok
set_jwt_issuer	jwt_issuer_present	ok
remove_jwt_issuer	jwt_issuer_absent	fails if issuer missing
set_jwt_public_signing_keys	jwt_public_signing_keys_present	ok
trust_node	node_trusted	ok (present happens automatically on join)
retire_node	node_absent	fails if node already retired
new_node_code	node_code_present	fails if code id already exists
retire_node_code	node_code_absent	fails if code id already retired (removed)
accept_recovery	recovered	awkward on a new network, can hopefully be combined with opened below
open_network	opened	fails if network already open
rekey_ledger	ledger_rekeyed	ok
update_recovery_shares	recovery_shares_refreshed	ok
set_recovery_threshold	recovery_threshold	ok

[lynshi]

In our recent API review, we were recommended to return 204 for deleting role assignments on nonexistent users instead of 404, because even if the user doesn't exist we've satisfied the intent of the operation. I also found a bunch of debate online whether to return 204 or 404 in this situation; I don't think there's a rigid standard here, so it's difficult to assume what users would expect.

I do like the idea of a boolean tuple returned by the methods ((<ok: true/false>, <changed: true/false>)) so that the developer has more information to use in their script. However, I don't like the present tense either because it still sounds like querying. Maybe this is just me.

Another scenario just came to mind. If, in the future, you want to provide methods for querying state to use in proposals so users can branch based on state (I understand this is generally just a convenience wrapper around writing table queries, so you can also imagine users defining their own functions to reference in their proposals?), naming the proposal methods with noun+state could make the query methods harder to name, or cause confusion about which method to use.

if is_network_open():  # Trust node 1 only if the network is open, but this is named very similarly to the proposal function network_is_open
    trust_node(1)

In general, I don't think naming with noun+state sufficiently implies that calling the method will update the state. Most resources I've seen online, as well as Swift's design guidelines, suggest naming methods with side effects as imperative verb phrases. If this helps us agree that there should be a verb in the method name, maybe we can come up with better verb-based descriptions? For example, delete_user_if_exists implies deleting an existing user and noop otherwise. Though this can lead to rather annoying names like open_network_if_not_open, so I still lean towards the short names like open_network.

[achamayou]

I am surprised by that, 204 No Content is to be returned "if the action has been enacted and no further information is to be supplied", and this is a situation where nothing is enacted. I could not find a specific statement in the S3 or Azure storage docs about this, but the other two APIs I have checked clearly return 404 in this situation:

• https://cloud.google.com/storage/docs/xml-api/delete-object
• https://docs.couchdb.org/en/stable/api/server/configuration.html#delete--_node-node-name-_config-section-key

Anyway, if noun_is_(adjective|past participle) is ambiguous with an is_noun_(adjective|past participle) boolean check, what about noun_becomes_adjective? network_becomes_open, user_becomes_present, node_becomes_absent?

I agree that Swift's method naming convention is clear (and we happen to do exactly that in our code by and large). Methods however tend to be specific about pre-conditions, and container interfaces will typically not treat deleting a non-existent item the same way as one that was there. Some verbs imply idempotency, like 'set_', while others hint at a precondition ('retire_', 'remove_', 'trust_' etc).

In clear cases of repetition with the same arguments, removing pre-conditions is inconsequential, but set_user(user_cert, user_attr1) followed by set_user(user_cert, user_attr2) will result in user_attr1 being overwritten. If the caller is hoping to set a particular attribute only without modifying the rest accidentally, they will need to write a comparison to what they currently believe the attributes to be to avoid this happening. To be clear, this is the current behaviour for this particular call, but I am not convinced it is ideal.

If everyone is happy with keeping the current names, I will not insist. My worry is that we may not be striking the right brevity/precision tradeoff here, considering the impact and otherwise inevitably verbose nature of proposals and votes.

[lynshi]

It's possible the 204/404 suggestion may have been specific to how we presented the particular endpoint. Anyway, that doesn't really matter.

noun_becomes_adjective is palatable in the cases you've mentioned, but I'm not sure if that can be evenly applied. set_user_data to user_data_becomes_set / user_data_becomes_present does not sound good.

I do see your point for why the existing names may be confusing or undesirable, but I also don't think we've come across a better alternative yet. With regards to the set_* concern, maybe use patch_* and have the behavior be like a JSON merge PATCH?

Since we are moving to JS, would we have more flexibility in the method signatures? Then we could do something like retire_node(<args...>, or_noop=True), where or_noop=True induces a noop if the node is unknown, and the user can specify noop=False to error if the node is unknown.

[letmaik]

current idea: keep names roughly as-is and add extra parameter skip_if_done for proposals that support non-idempotent behaviour.

[lynshi]

Just documenting a few additional points from today's discussion.

• Main directions explored
  • Function parameter to configure idempotency, like the aforementioned skip_if_done.
  • Two flavors for each method that have implied preconditions (so excluding functions like set_*), such as a retire_node and retire_node_if_exists.
• We settled on using the parameter, so the challenge is naming the parameter appropriately.
• We are focusing on consistency and clarity for naming. In general, we prefer a ubiquitous name over disparate keywords applied to specific functions.
  • The next best alternative for the parameter name was idempotent to configure the idempotency, but we felt that might be harder to understand.
  • We considered words indicating presence (e.g. if_exists and if_not_exists) for the argument name, but that's not applicable to functions like open_network(), where the parameter would have to be if_not_open, so this approach doesn't easily satisfy consistency.
  • Existing patterns from other tools were discussed, like --force or --skip-checks. In general, such names seem to be too broad and can imply greater consequences than we intend; for example, we'd have to clearly document the checks --skip-checks is bypassing.

[letmaik]

After more discussion with @achamayou we came up with the following:

Idempotency:

• Everything is idempotent (except Actions, which cannot be, see below)
• No extra argument to control idempotency
• Customization can be done by changing/extending the to-be-JS constitution

Naming rules:

• Maps: (set|remove)_*
• Maps that have complex values: patch_* with JSON Merge Patch payload
  • optional if not useful
• Sets: (add|remove)_*
• Actions (non-idempotent): trigger_*
• State transitions: transition_*

Map-based proposals require a key.
Set-based proposals add/remove from a set.
Actions always trigger an operation. They are idempotent from a result point of view since they can be repeated and will not fail. But they are non-idempotent in the conventional meaning since they do run operations repeatedly and will change the state repeatedly.
State transitions are based on transitions in a state machine.

old name	new name	notes
set_js_app		REMOVE
deploy_js_app	set_js_app
remove_js_app	(unchanged)
set_module		REMOVE
remove_module		REMOVE
new_member	set_member	requires member ids to be hash-based
retire_member	remove_member	currently fails if member missing or not active
n/a	patch_member	NEW
set_member_data		REMOVE
new_user	set_user	currently fails if user exists; requires user ids to be hash-based
remove_user	(unchanged)	currently fails if user missing
n/a	patch_user	NEW
set_user_data		REMOVE
set_ca_cert_bundle	(unchanged)
remove_ca_cert_bundle	(unchanged)
set_jwt_issuer	(unchanged)
remove_jwt_issuer	(unchanged)	currently fails if issuer missing
set_jwt_public_signing_keys	(unchanged)
trust_node	transition_node_to_trusted	there is no add_node as nodes are automatically added when joining
retire_node	remove_node	currently fails if node already retired
new_node_code	add_node_code	currently fails if code id already exists
retire_node_code	remove_node_code	currently fails if code id already retired / removed
open_network	transition_network_to_open	currently fails if network already open
accept_recovery		REMOVE and combine with open_network
rekey_ledger	trigger_ledger_rekey
update_recovery_shares	trigger_recovery_share_refresh
set_recovery_threshold	(unchanged)

[achamayou]

@lynshi can we get your feedback on this proposal?

[lynshi]

This looks good to me; I like having the set of prefixes to help distinguish the operations.

One question - would remove_node simply retire the node, or remove it from the set completely? The naming implies the latter, so a list of network nodes wouldn't include the removed node anymore.

I just thought of this because, previously, /node/network_info would only return a list of trusted nodes; however, we've noticed the new /node/network/nodes returns all nodes, even retired ones, unless a filtering query parameter is passed. So, retiring a node vs removing a node from the set of nodes seems to mean slightly different things.

[achamayou]

remove_node will actually remove the node from the set completely, we do not want a forever growing table of retired nodes. Querying removed nodes will require a historical query after this change.

/node/network/nodes will still have a filtering query to distinguish trusted/pending nodes.