Hip-904 account changes supporting pending airdrops

[ibankov]

Description:

Proto changes supporting account relations to pending airdrops

Related issue(s):

Fixes #

Notes for reviewer:

Checklist

• Documented (Code comments, README, etc.)
• Tested (unit, integration, etc.)

[vtronkov]

Except Michael's comment: LGTM

[jsync-swirlds]

The copyright header copied from other files is incorrect, and most of the specification comment wording could use significant improvements.
Small note, the renumbering of Enum entries is permissible in this case, but we should be extremely careful not to do so for any values that are already merged to the main branch.

I've also raised a general concern about the increased state size elsewhere.

[jsync-swirlds]

This is purely duplicative. The pending airdrop ID above can be used to look up the pending airdrop value, and should be. Duplicating the value here just increases the number of places that need to be updated and increases the chance that we make a mistake.

[Neeharika-Sompalli]

Agreed we don't need the value here.

[ibankov]

We need this because we are we are changing the state map to be <PendingAirdropId,AccountAirdrop>, do you suggest that we store the amount directly as unit64

[jsync-swirlds]

Are you saying this will replace the existing PendingAirdropValue message in state?
If we're doing that, the HIP needs to change (which should be discussed across teams and include external stakeholders) to include this message, as that is no longer just an "internal" detail.

The existing PendingAirdropValue message only exists to hold a single uint64 value; if we're replacing that message with this one, then we wouldn't need the wrapper and could just store the primitive value here.

[netopyr]

This changes only the internal representation, which is an implementation detail. The HAPI messages should not derive from the HIP. Why do you think it needs to be discussed across teams? Does the HIP define how the information is stored in state?

[jsync-swirlds]

It does, it describes the storage of pending airdrops as a map from PendingAirdropId to PendingAirdropValue. We can perhaps debate the wisdom of doing so some other time.

I'm not intending to imply we cannot make a change, only that it probably needs some additional discussion at this stage.

The growth of state for pending airdrops was discussed in the HIP review quite a bit, and there was a significant concern that the storage be as small as possible. Whatever changes are made here should probably keep that in mind.

It is, perhaps, worth noting that in-state representation probably ceases to be a purely internal detail with Block Stream. Block Streams are required (as I understand it) to include state changes with sufficient detail that the entire state can be reconstructed on a Block Node, which implies all representations in state affect the Block Stream and future Block Nodes. Any client of a Block Node could probably be affected as well (depending on what that client used from the stream).

[ibankov]

We will look into updating the HIP, as for this value I think we need to use PendingAirdropValue and not the unit64 as we use PendingAirdropValue in the record file as well.

[jsync-swirlds]

It's not a critical point, but there is nothing that requires us to directly match values in state with values in a record file.
As long as records can be created (and we're just passing a Java long to the record builder to do that) we can store things in state very differently from the record files.