[Malleability] Refactor BlockDigest and ChannelList types

[UlyanaAndrukhiv]

Closes: #6665, #6667

Context

This pull request addresses two issues related to the misuse and redundancy of the ID() method in BlockDigest and ChannelList.

• BlockDigest: the id field has been renamed to BlockID, and the ID() method has been removed. All affected unit tests and usages have been updated to reflect this change.

• ChannelList: the ID() method has been removed, and all relevant tests have been verified to ensure correctness.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 41.14%. Comparing base (91bef99) to head (128d875).

Files with missing lines	Patch %	Lines
access/handler.go	0.00%	1 Missing ⚠️
model/flow/block.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feature/malleability    #6980      +/-   ##
========================================================
- Coverage                 41.15%   41.14%   -0.02%     
========================================================
  Files                      2122     2122              
  Lines                    187214   187209       -5     
========================================================
- Hits                      77055    77020      -35     
- Misses                   103718   103744      +26     
- Partials                   6441     6445       +4     

Flag	Coverage Δ
unittests	41.14% <33.33%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[durkmurder]

I think this pattern is a bit of a bad practice. Specifically it assumes that we first create an object which is in "invalid" state and then call Build to initialize it. I am a big fan of programming idiom where we construct objects that hold their invariant. This means that we create objects in valid state at initialization.

Current usage of Build:

var block models.BlockDigest
block.Build(blockDigest)

proposed solution:

block := ConvertFromBlockDigest(blockDigest)

Additionally, I would consider getting rid of Build all together. It has several usages that seem to be redundant.

[AlexHentschel]

Fully agree with Yurii's guidance for the protocol-level: we highly value correctness and therefore should be embracing design patterns that are "safe by default" - in other words, patterns that are very hard to accidentally use them the wrong way.

Though, this "builder pattern" seems to be very broadly used only in the Access API, to convert flow-internal data types to json-encodable types that can be sent over REST, for example:

flow-go/engine/access/rest/common/models/block.go

Lines 30 to 31 in e027b13

	var payload BlockPayload
	err := payload.Build(block.Payload)

Given how prevalent this pattern is, I would be inclined to not make changes as part of the malleability work-stream. Within the malleability work-stream, I don't think we are going to be able to notably reduce the prevalence of this pattern ... but by partially "fixing" this pattern here and there, we would be making the code less consistent, which also has its downsides.

Nevertheless, please lets make sure we are not introducing this pattern into business logic of the lower-level protocol. I can live with it within the Access API, but not within the code guaranteeing correctness of the protocol overall. If we need a builder within the lower-level protocol, please make sure the builder is a distinct data structure that is different from the struct which the builder is producing. Then, there is no risk of accidentally conflating a builder in an interim incomplete state with a properly constructed output of the build process.

[durkmurder]

Nice and clean. 🎸