SaitoTech / saito-rust

A high-performance (reference) implementation of Saito in Rust
42 stars 10 forks source link

Naming Conventions #69

Open BearGuy opened 3 years ago

BearGuy commented 3 years ago

There's been some discussion about how we name things in the codebase, so I wanted to open up a discussion so that we could stick to some common conventions.

By default, we match the Rust styling: https://doc.rust-lang.org/1.0.0/style/README.html

In general, we should fully type all names unless the name runs significantly long. We don't cut corners by shortening variables or attributes, so block over blk and transaction over tx. In the future, well defined acronyms can describe processes like automatice transaction rebroadcasting (ATR for short).

Getter functions just reference the piece of data that we want access to -> id() returns id. This also applies to a function like latest_block in Blockchain.

Setter functions use the convention of set_${attribute}.

Additional functions that perform a more complex transform, or some other complex functionality will start with a verb, then describe what the function does. If there's not a guarantee that the operation will be successful, we use the verb try. Only if we're sure of the success of the operation would this be negated.

Here's an example with Blockchain. When we want to add a block to the blockchain, the function we call is try_add_block, and not add_block. The function add_block is only called when we are certain that the block should be added to the blockchain. In a similar vein, when we attempt to bundle a block in Mempool we call try_bundle_block, and only call bundle_block when the proper requirements are met.

For our primitive structs, we want to distinguish between Primitive and PrimitiveCore. Core version of the struct will be what's sent over the wire between nodes, and represents the simplest version of the struct. The full-bodied version is available to allow for each node to enrich its data.

trevelyan commented 3 years ago
  1. General Naming Approach:

I am comfortable with this approach. Would suggest variable names also reflect the organizational logic of their most generic high-level container. This gives us [ block, block_id, block_hash, transaction_id, transaction_msg ] rather than [ block, block_core_id, block_core_hash, transaction_core_msg ]. The parent class Block and Transaction can handle access to their internal Core structs as needed and hide that complexity for us. This also encourages us to pass around the parent objects and buffers us to arbitrary changes in organizational structure that will result in variables like [ previous_block_header_index ].

  1. Very Specific Suggestions:

Suggest use "publickey" instead of "address" or "public_key". Am curious if people want _varname or varname for variables that are only used internally. Happy either way but think we should be consistent.

  1. Thoughts on Try:

This seems obviously useful for functions that throw an error. But for many functions the fact that they end in one state or the other or return 1 or 0 does not imply that they have failed (slip.validate() / slip.try_validate()). If we don't need error-catching wrapping around the function do we really need a reminder that different things might happen? I am concerned this "sounds good" but will result in arbitrary linguistic concepts of what constitutes success/failure becoming the guideline instead of much more narrow technical questions of "can this error out".

Also suspicious as I think we need a minimal set of functions that creates a common language to reason correctly about consensus:

add_block add_transaction add_slip validate serialize deserialize

Many of these functions should exist in mempool, transaction, block and blockchain. I suspect that if we are using the "will this fail" approach we will end up with inconsistencies and half of our functions end up being validate() and half will be try_validate(). Knowing that validate() always return 0 or 1 is conversely quite powerful.

escapedcat commented 3 years ago

Is this solved? Could be closed I guess.