Proto: split `delegate_storage.ml`

While trying to understand the delegation and staking code, I found the file delegate_storage.ml a bit harsh to grasp. It is a huge file: ~1000 lines. I tried to split the file in multiple files, with separate concerns to guide my understanding. Just like there is already smaller files like delegate_activation_storage.ml responsible for only one or two keys in the store (for instance delegate_activation_storage.ml manages Storage.Contract.Delegate_last_cycle_before_deactivation and Storage.Contract.Inactive_delegate).

Is there any interest for such a refactoring?

Here is the proposed split:

• delegate_storage.ml
  • Storage.Delegates
  • Storage.Contract.Frozen_deposits_limit
• delegate_missed_endorsements_storage.ml
  • Storage.Contract.Missed_endorsement
• delegate_slashed_deposits_storage.ml
  • Storage.Slashed_deposits
• delegate_sampler.ml
  • Storage.Delegate_sampler_state
• delegate_cycles.ml
  • cycle_end and init_first_cycles

The MR also contains eight related minor refactoring, which simplify some interfaces:

• Proto: move set_active into Stake_storage

  It merges Delegate_table_storage.set_active into Stake_storage.set_active. The function in Delegate_table_storage, do not require access to the part of Storage from which Delegate_table_storage is responsible. Merging the two function simplify the interfaces and avoid having three distinct set_active functions.

• Proto: Rename Delegate.check_delegate into Delegate.check

  Just a renaming.

• Proto: simplify Delegate.registered

  The function Contract_delegate_storage.registered is based on invariant ensured by Delegate_table_storage (i.e. a delegate is self delegated), better to move the function. Then, we simplify the function implementation by requiring only one read access into the context.

• Proto: remove freeze_deposits_do_not_call_except_for_migration

  This function is only called once in the whole protocol code. I suggest to inline it for avoiding incorrect usage. Besides, the name is a bit misleading, the function is only used while migrating from genesis, which is more a chain initialization that a protocol migration.

• Proto: merge Seed.cycle_end into Delegate.cycle_end

  It is to enforce a little bit more the following comment in seed_storage.ml: (* NB: the clearing of past seeds is done elsewhere by the caller *).

• Proto: introduce Alpha_context.Contract.Delegate

  Move the functions Alpha_context.Delegate.{init,set,find} into Alpha_context.Contract.Delegate. These functions basically handle the field 'delegate' of a contract. They do not handle storage related to a given delegate, like other function in Alpha_context.Delegate.

• Prote: rename Delegate.delegate_participation_info into Delegate.participation_info

  Just a renaming.

• Proto/Delegate: remove dead code

  Remove the duplicate function Stake.delegate_pubkey (also available as Delegate.pubkey)

added 3 commits

• 10dad4ff - Proto: split `Delegate_storage` in multiple files
• 29c8b704 - Proto: merge `Seed.cycle_end` into `Delegate.cycle_end`
• 1e7a58d0 - Proto: remove `freeze_deposits_do_not_call_except_for_migration`

Compare with previous version

changed the description

added 3 commits

• 4accd84c - Proto: split `Delegate_storage` in multiple files
• 1ad35d52 - Proto: merge `Seed.cycle_end` into `Delegate.cycle_end`
• c4fa4b94 - Proto: remove `freeze_deposits_do_not_call_except_for_migration`

Compare with previous version

Thank you for your contribution , I have a couple of remarks:

• There are a few touched files that contain nonstandard "TODO" and "FIXME" comments:
  • → apply.ml
  • → init_storage.ml
  • → plugin.ml
  • → … and there are 3 more …
  • Please help stadardize these as described in the tezos documentation.
• There are changes to proto-alpha:
  • → src/proto_alpha/lib_protocol/{TEZOS_PROTOCOL,alpha_context.ml,alpha_context.mli, …}
  • → Please make sure changes are properly tagged Proto: and protocol reviewers are alerted.
• There are changes to protocol-client-code:
  • → src/proto_alpha/lib_client_commands/client_proto_context_commands.ml, src/proto_alpha/lib_delegate/test/tenderbrute/lib/tenderbrute.ml
  • → Please make sure changes are properly replicated for all relevant protocols (e.g. proto_alpha, proto_010_PtGRANAD, proto_011_PtHangz2).

---

I,  , ran this inspection on Fri, 15 Apr 2022 06:30:56 +0000. I usually run inspections every 6 hours and will try to edit my own comment if I have any updates. For more information see the Tezos documentation.

Thank you for your contribution , I have a couple of remarks:

• There are a couple of touched files that contain nonstandard "TODO" and "FIXME" comments:
  • → apply.ml
  • → init_storage.ml
  • → test_delegation.ml
  • Please help stadardize these as described in the tezos documentation.
• There are changes to proto-alpha:
  • → src/proto_alpha/lib_protocol/{TEZOS_PROTOCOL,alpha_context.ml,alpha_context.mli, …}
  • → Please make sure changes are properly tagged Proto: and protocol reviewers are alerted.
• There are changes to protocol-client-code:
  • → src/proto_alpha/lib_client_commands/client_proto_context_commands.ml, src/proto_alpha/lib_delegate/test/tenderbrute/lib/tenderbrute.ml
  • → Please make sure changes are properly replicated for all relevant protocols (e.g. proto_alpha, proto_010_PtGRANAD, proto_011_PtHangz2).

---

I,  , ran this inspection on Thu, 12 May 2022 12:38:45 +0000. I usually run inspections every 6 hours and will try to edit my own comment if I have any updates. For more information see the Tezos documentation.

requested review from @eugenz

requested review from @bsall

Assigning @eugenz for the first round of review.

assigned to @eugenz

added Code quality 🌟 proto labels

Overall this MR makes some useful refactorings.

However, as far as I see, in the main commit, there are other refactorings being made besides moving code in different modules, for instance:

• the new delegate_public_key type in Raw_context
• moving some functions from Alpha_context.Delegate to a different module
• removal of some functions do_not_call_except_for_migration

I think it would help to put independent concerns in separated commits, both for clarity and for ease of review. Ideally, the commit that aims to move code around should do only that.

Also, writing a small motivation for the refactoring (when possible) would also help. For instance, I have some doubts about including Seed.cycle_end in Delegate.cycle_end, as seeds are an independent notion from delegation. (Though it's true that snapshots are treated in Delegate and also an independent notion...)

assigned to @g.b.fefe and unassigned @eugenz

added 164 commits

• c4fa4b94...0f75e2b4 - 157 commits from branch tezos:master
• c8786f08 - Proto: split `Delegate_storage` in multiple files
• 6f6e071f - Proto: move `set_active` into `Stake_storage`
• de51a2e7 - Proto: Rename `Delegate.check_delegate` into `Delegate.check`
• 15c0f0d0 - Proto: simplify `Delegate.registered`
• 8918001f - Proto: remove `freeze_deposits_do_not_call_except_for_migration`
• d44ca9fe - Proto: introduce `Alpha_context.Contract.Delegate`
• 5aed2c65 - Proto: merge `Seed.cycle_end` into `Delegate.cycle_end`

Compare with previous version

changed the description

added 41 commits

• 5aed2c65...aabc0bd1 - 34 commits from branch tezos:master
• bd579a5c - Proto: split `Delegate_storage` in multiple files
• 1c782d97 - Proto: move `set_active` into `Stake_storage`
• 7c38abb3 - Proto: Rename `Delegate.check_delegate` into `Delegate.check`
• 60ef1581 - Proto: simplify `Delegate.registered`
• 758d1394 - Proto: remove `freeze_deposits_do_not_call_except_for_migration`
• 41b6b2dc - Proto: introduce `Alpha_context.Contract.Delegate`
• b9e01043 - Proto: merge `Seed.cycle_end` into `Delegate.cycle_end`

Compare with previous version

assigned to @eugenz and unassigned @g.b.fefe