Bond event readability
I am writing here some aspects of bond event readability that I believe are worth improving.
[Update: The BondType strangeness I now believe is due to using string(m.BondType)
(for the enum
field), in contrast with EventMintBurn's m.Supply.String()
or EventPendingLiquidity's m.PendingType.String()
, both of which are displayed normally.
|
Perhaps the approach of NewEventSwitchV87
, which creates an a EventSwitchV97 event of identical type to the original EventSwitch. I think I can also see how to make the Midgard code interpret correctly both old and new formats, using its PendingType code as a reference. I would however also like to add unambiguous NodeAddress and BondAddress fields (interpreted by Midgard) to an EventBondV, for which further study of how EventSwitchV87 added fields is appropriate.
|
Alternatively, the next hard fork could be an opportunity to change EventBond details without adding a version identifier. (Perhaps a version identifier would still be convenient as a clear indicator of when its structure changed.)]
See this Discord thread.
-
"bond" event "bond_type" can have the four types of bond_paid, bond_returned, bond_reward, and bond_cost. However, a BondCost bond_type for instance is displayed in the form "\u0003", which must be looked up to be interpreted, rather than read at a glance.
-
The same bond address can be and often is associated with multiple node addresses, with different bond amounts for each node. As such, to be understandable a bond event must clearly list both the bond address and node address.
Otherwise, if only the bond address is listed, all nodes with that bond address must be compared (looking at the bond amounts for the blocks before and after/during the bond event) to determine which bond position it refers to.
Notably, at present the "bond" event structure has "from" and "to" keys, but no "node_address" or "bond_address" keys for which the meaning would be clear at a glance.
As a further note, when a slash BondCost event were emitted, the Asgard or Yggdrasil vault address for which the event were being emitted would be welcome, but less important than having both the node and bond addresses.
|
Edit: Notably, currently a bond event 'type' comprises an amount, bondType, and TxIn. At least for a BondCost, a "reason" key could be convenient for distinguishing between the many reasons for a BondCost. -
tx.ToAddress = common.Address(nodeAccount.String())
should be something else, as currently the appearance is incredibly cluttered (as well as being unintuitive).
Second edit: Image replaced with more recent examples from Block 6460294 (2022-07-16) and Block 6508740 (2022-07-20).
Update: Please see this summary of all instances of NewEventBond
calls, as of network version v1.102.0 .
2023-06-13 update:
I am still in favour of switching from string()
to .String()
for the bond type, but am leaning more in favour of standardising the to
and from
fields to have different usages depending on the bond type. For instance a BondCost (a slash) which is taken from a node's total_bond
rather than a specific bond provider could have an empty to
(since going to both pool and Reserve) and the node address for the from
,
whereas a BondReturned (bond-provider-specific unbond) could have the node address as the from
and the bond provider's bond address as the to
, thus providing full information on the circumstances.
(For a BondReturned, a bond provider could be a provider for multiple nodes and a node could have multiple providers, thus only by specifying both is the entire flow of funds communicated.)
2023-09-17 update:
I used to think it would be necessary for backwards compatibility to emulate the introduction of EventSwitchV87
in changing emitted event state.
However, with !3038 (merged) as an example (type_events.proto
, type_event.go
my current impression is that the bond event change can be done as a refactor,
though as a consequence retroactively changing Midgard events
(for instance making historical blocks' bond_type
more readable).
I also note the comment here for !3108 (merged) about how an added protobuf string field (to the end of the fields rather than replacing an earlier one) is effectively nullable, not causing earlier KVStore issues by being empty.
(Noting however, as it bears repeating, the distinction with how an 'empty' Uint when cast to the string is "0"
, distinct from the empty string ""
.)
Midgard code would as expected still need modification to be able to interpret the changed bond_type
and addition of node_address
and bond_address
fields.
As an incidental comment, bond_cost
NewEventBond
calls would perhaps have the AccAddress equivalent of NoAddress (""
) for the bond_address
where a cost were shared by all bond providers, and the bond provider's bond_address
if a cost were only from a single bond provider.
(I have not yet reread whether both cost types exist.)
|
BondProvider bond_address
is of type AccAddress, as is NodeAccount node_address
, so if convenient I am inclined for added NewEventBond
arguments to be AccAddress.
A question there is what to do if the BondProviders slice is empty, with only type Address bond_address
(displayed as node_operator_address
) available.
Perhaps Address-received AccAddress
could be used there,
with nil
for when no bond address to be specified.
My current code proposal is !3138
'[Retroactive event changes] Clarify bond events #check-lint-warning'.
Relevant:
https://github.com/cosmos/cosmos-sdk/blob/v0.45.1/types/address.go#L264-L266 \
func (aa AccAddress) String() string {
if aa.Empty() {
return ""
https://github.com/cosmos/cosmos-sdk/blob/v0.45.1/types/address.go#L188-L189 \
func (aa AccAddress) Empty() bool {
return aa == nil || len(aa) == 0
(Pleasing in that expecting .String()
of a nil
AccAddress to produce an empty string.)