InfractionCollector Contract

[theref]

Scope is outlined in #266

• First Draft of Contract
• Updates to existing contracts
• Deployment scripts
• Tests

Related to https://github.com/nucypher/sprints/issues/35

[cygnusv]

Looking good!

[KPrasch]

What is the reason why the top-level index here is the ritual ID? I'm not suggesting any changes at this moment but how this data structure is formed has implications on how cost-effective and code-efficient looking up infractions is later.

[cygnusv]

Each storage lookup has a relatively high cost, so in general, you'd want to reduce the number of read and writes. In this case, there's some optimizations you can do here:

• If for each ritual and staker there can only be one infraction (which doesn't seem unlikely), you can remove the last bool since storage slots are falsy by default (i.e., zero bytes). For that you'd only need to add a NONE initial value to InfractionType, so that actual infractions write non-zero bytes.
• An alternative implementation choice that is often used in smart contracts is to use a hash-based look-up key, instead of a chain of mappings. See e.g., https://github.com/nucypher/nucypher-contracts/blob/main/contracts/contracts/coordination/GlobalAllowList.sol#L38-L44
• If you prefer to keep the mapping chain, Solidity supports naming the keys and values in the mapping definition, so it'd be something like: mapping(uint32 ritualId => mapping(address stakingProvider => mapping(InfractionType => bool))) public infractions. This is just a readability improvement.

[derekpierre]

how this data structure is formed has implications on how cost-effective and code-efficient looking up infractions is later.

This is a really interesting consideration.

I don't have clear thoughts on this at the moment, but is it more likely that infractions would be looked up/checked via staking provider address instead of by ritual id? This would be from a dashboard or ...(?).

For example, would we want to know all infractions for a specific staker address as opposed to all infractions from a failed ritual id? Even in the latter case (all infractions from failed ritual id), it is easy to get the list of all staking provider addresses for a ritual id from the coordinator (which this contract also has a reference to), and then check infractions for each staking provider address.

So, keying by staking provider address instead of ritual id could be better.

[KPrasch]

Related to my above comment - it's worth considering an alternate structure here so that it's easier to lookup all infractions pertaining to a single node. Without any real world usage data, it's hard to know exactly what types of on-chain queries we will need to handle penalties later. Based on prior discussions, we may need to identify multiple subsequent infractions in order to fairly enforce a downtime penalty.

[vzotova]

taco app is checking subsequent penalties, so we can put it there, otherwise structure will change to mapping and arrays and will consume more gas (not a end of the world though)