Skip to content

Conversation

tdelabro
Copy link
Collaborator

@tdelabro tdelabro commented Sep 16, 2025

Add a ContractAddress wrapper type that guarantee the felt it contains is a valid starknet contract address.
Contain some quality of like function for string conversion

Not a breaking change

Discussion:
@xJonathanLEI suggested we add ClassHash too
I chose to exclude addresses 0x0 and 0x1 from the normal ContractAddress flow. You can still build them with the unchecked one. Should we add constant ContractAddress::ZERO and ContractAddress::ONE to make those special case more explicit?
Do we want to allow them at all? Is there a risk to make some API unusable downstream if those value are entierly excluded?

@tdelabro tdelabro force-pushed the add-contract-address-type branch from bea90b3 to 02d6946 Compare September 16, 2025 15:15
Copy link
Collaborator

@gabrielbosio gabrielbosio left a comment

Choose a reason for hiding this comment

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

Some small comments

/// * 0x1 functions as a storage space for block mapping [link](https://docs.starknet.io/architecture-and-concepts/network-architecture/starknet-state/#special_addresses)
/// - They must be less than 2^251 (0x800000000000000000000000000000000000000000000000000000000000000)
///
/// Making the valid addressabe range be [2, 2^251)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Making the valid addressabe range be [2, 2^251)
/// Making the valid addressable range be [2, 2^251)

)
}
ContactAddressFromFeltError::TooBig => {
write!(f, "the highest possible address is 2^251 - 1")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also add the contract address value in the error message and replace 2^251 - 1 with the value of ADDRESS_UPPER_BOUND so we can more easily compare the failing contract address with the upper bound

@gabrielbosio gabrielbosio self-requested a review September 23, 2025 21:50
/// which would result in permanent loss.
impl Felt {
pub fn is_valid_contract_address(&self) -> bool {
self >= &Felt::from(2u64) && self < &ADDRESS_UPPER_BOUND
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self >= &Felt::from(2u64) && self < &ADDRESS_UPPER_BOUND
self >= &Felt::TWO && self < &ADDRESS_UPPER_BOUND

@tdelabro
Copy link
Collaborator Author

@gabrielbosio do you have an opinion about the points I highlighted in the issue description?

@gabrielbosio
Copy link
Collaborator

@gabrielbosio do you have an opinion about the points I highlighted in the issue description?

It depends if we want to migrate the ContractAddress struct from the sequencer repo. If that's the case then the try_from method should only check for the upper bound like it's done here.

@tdelabro
Copy link
Collaborator Author

@dorimedini-starkware you are maintaining the sequencer repo?
What do you think?
Is there a reason not to have a common ContractAddress shared between the whole ecosystem (sequencer, starknet-rs, ...)

@dorimedini-starkware
Copy link
Collaborator

@dorimedini-starkware you are maintaining the sequencer repo? What do you think? Is there a reason not to have a common ContractAddress shared between the whole ecosystem (sequencer, starknet-rs, ...)

no reason I can think of, but this suggested implementation differs from ours in several ways, like:

  1. we have another indirection (ContractAddress(pub PatriciaKey(Felt)) vs. ContractAddress(Felt))
  2. we allow zero and one as valid addresses
  3. we use derive_more::Display and derive_more::Deref
  4. serde derives are not feature-gated

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.

3 participants