Remove unnecessary PhantomData usage

[MaksymZavershynskyi]

PhantomData has a very specific use case -- to work around issues with the compiler checks: https://doc.rust-lang.org/nomicon/phantom-data.html

Currently, we are using it in some places to prevent initialization of the struct with curly braces {}, e.g.: https://github.com/nearprotocol/nearcore/blob/3cab57953dbaf1f89c5f6026aa121ed519b50c73/core/chain-configs/src/genesis_config.rs#L109 It is actually not important that we are using PhantomData in there. We could be using any type as long as borsh and serde skip them and they have default initializer.

Instead, we should prohibit usage of {} through the visibility, e.g. by making some or all fields of Genesis non-public and providing accessor-methods. We also can potentially avoid accessor methods by using pub(self) or similar see: https://doc.rust-lang.org/reference/visibility-and-privacy.html#pubin-path-pubcrate-pubsuper-and-pubself

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[matklad]

Hm, this specific example doesn't make sense to me:

#[derive(Debug, Clone, Default, Serialize, Deserialize)]
pub struct Genesis {
    #[serde(flatten)]
    pub config: GenesisConfig,
    pub records: GenesisRecords,
    /// Using zero-size PhantomData is a Rust pattern preventing a structure being constructed
    /// without calling `new` method, which has some initialization routine.
    #[serde(skip)]
    phantom: PhantomData<()>,
}

This struct implements Serialize and Deserialize, which is an even stronger property than just making every field pub. If the structure is trivially serializable, then the client can create an arbitrary instance, without checking any invariants.

In other words, gettres and private fields are useless for derive(Serialize) structures.

Not sure what's the right fix here, I haven't looked deeply at the code yet.

[nikurt]

The other 2 use cases of PhantomData are:

• https://github.com/near/nearcore/blob/7ed1ee5f6aabb62914c7fb06aabfe1e2aad725cf/runtime/near-evm-runner/src/pricer.rs#L397
• https://github.com/near/nearcore/blob/7ed1ee5f6aabb62914c7fb06aabfe1e2aad725cf/utils/near-performance-metrics/src/framed_write.rs#L408

Both of these use cases are justified. The first one matches the use case from the PhantomData documentation: Unused Type Parameters Less obvious why the second use case needs PhantomData. Seems to be the glue between the application logic and the actix framework, while also owning an usused type parameter of the Actor.

[nikurt]

Genesis::new maintains the following invariant: Genesis.genesis_config.total_supply is equal to the sum of amount and locked from Genesis.records. In a sense, total_supply is constant. As @matklad noticed, this invariant can now be violated by deserializing a config with an arbitrary total_supply value. On the other hand, records don't have to be loaded in memory and the code doesn't run for_each_record on load. We could either:

• skip deserializing Genesis.genesis_config.total_supply and compute it from the records
• replace total_supply field with a method and compute it from the records

but there are cases, such as testing of GenesisConfig logic, where the records are unavailable and deserialization of total_supply is needed.