Skip to content

refactor: remove the Addr and Hash generics in consensus #2855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rianhughes
Copy link
Contributor

@rianhughes rianhughes commented May 27, 2025

Summary

The consensus implementation currently defines Hash and Addr as generic interfaces to support flexibility across protocols. This was originally intended to allow the consensus logic to work with any hash and address types that can be represented as [4]uint.

However, in the context of the Starknet protocol, both the address and hash types are explicitly defined as felt.Felt, and this is not expected to change. As a result, the use of generics here is unnecessary and introduces avoidable complexity.


Motivation

  • Unnecessary Boilerplate: Since Hash and Addr are used pervasively across the consensus package, requiring them to be passed explicitly as generic parameters to every struct and function results in significant boilerplate and reduces readability.

  • Interface Overhead: Defining these as interfaces requires explicit type declarations, type assertions, and additional complexity that brings no real benefit in this fixed-type context.

  • Improved Ergonomics: By redefining Hash and Addr as aliases (e.g., type Addr felt.Felt), we significantly reduce syntactic noise and make the code more straightforward and maintainable.


Change

This PR:

  • Replaces the generic Hash and Addr interfaces with type aliases to felt.Felt.
  • Updates the affected types and function signatures to remove unnecessary generic parameters.

This change simplifies the consensus implementation while preserving all intended functionality under the assumption of fixed felt.Felt-based types.


Next (this can be done in a separate PR)

Investigate whether the Hashable interface can also be removed by explicitly defining the concrete value types used in the Starknet protocol (e.g., Block, StateUpdate, etc.).

Since these types are known and fixed within the protocol, relying on an interface abstraction may be unnecessary. Replacing Hashable with concrete types could further simplify the codebase and eliminate another layer of indirection.

@rianhughes rianhughes marked this pull request as ready for review May 27, 2025 09:37
@rianhughes rianhughes requested review from infrmtcs and rodrigo-pino and removed request for infrmtcs May 27, 2025 09:37
Copy link

codecov bot commented May 27, 2025

Codecov Report

Attention: Patch coverage is 98.26590% with 3 lines in your changes missing coverage. Please review.

Project coverage is 74.77%. Comparing base (65fe7e9) to head (95c64f8).

Files with missing lines Patch % Lines
consensus/types/action.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2855      +/-   ##
==========================================
- Coverage   74.83%   74.77%   -0.06%     
==========================================
  Files         206      207       +1     
  Lines       22469    22473       +4     
==========================================
- Hits        16815    16805      -10     
- Misses       4598     4607       +9     
- Partials     1056     1061       +5     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines +6 to +8
type Hash felt.Felt

type Addr felt.Felt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we use existing types in core instead? That would reduce one layer of conversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, also happy to do that

}

type (
Address = address.Address
Hash = hash.Hash
Hash = types.Hash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we use existing hash type from core instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, also happy to do that

@infrmtcs
Copy link
Contributor

I'm against this change because:

  • Most of the implementation doesn't need to know the concrete type. Replacing generics by concrete types prevents the ability to write tests using other set of types. For example, if we can get a set of Tendermint tests from other implementations, there is no way to adapt to our code base except by manually rewriting using our types.
  • Regarding "Interface Overhead", I don't think this reduce any overhead, because we're not using any type assertion over generic type, and the performance shouldn't be affected by type instantiation.
  • The generic types help separate between the concrete implementation for Starknet and generic Tendermint logic. Inside Tendermint general package, we use generics. Outside it, we can use Starknet concrete types in consensus/starknet package.

@rianhughes
Copy link
Contributor Author

  • Most of the implementation doesn't need to know the concrete type. Replacing generics by concrete types prevents the ability to write tests using other set of types. For example, if we can get a set of Tendermint tests from other implementations, there is no way to adapt to our code base except by manually rewriting using our types.

Imo, the implementation doesn't need to work with an abstract type since it's directly tied to Starkenet. Also it's not clear to me what tests we would be taking from other implementations (that are compatible with Starknets implementation), and if we couldn't just write a simple adapter for this case (eg AdaptEthHashToFelt()). It just feels like over-engineering to me, but I may be wrong.

  • The generic types help separate between the concrete implementation for Starknet and generic Tendermint logic. Inside Tendermint general package, we use generics. Outside it, we can use Starknet concrete types in consensus/starknet package.

I agree this helps separate the Starknet types and the generic Tendermint logic. I just don't think it's worth it, particularly given Starknet is using their own custom Tendermint logic, not the vanilla implementation.

It might be worth getting a third. @rodrigo-pino what are your thoughts?

@infrmtcs
Copy link
Contributor

I just don't think it's worth it, particularly given Starknet is using their own custom Tendermint logic, not the vanilla implementation.

As far as I understand, the only changes in Starknet are not in the core part of the algorithm including state machine and driver, but instead in the communication part, namely proposal stream and re-broadcast.

@rianhughes
Copy link
Contributor Author

To do: investigate if we can remove the interfaces, but keep the Tendermint-Starknet abstraction layer

@rianhughes rianhughes marked this pull request as draft June 2, 2025 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants