Skip to content

Conversation

@Galoretka
Copy link
Contributor

@Galoretka Galoretka commented Oct 24, 2025

Contract.address could remain undefined or an empty string due to a weak null check and permissive constructor assignment, causing runtime errors in provider calls and when converting the address to hex; the constructor now always normalizes to lowercase and validates that the address is a non-empty string, all public methods that require a valid address (call invoke estimate isDeployed) now guard with a strict string check, and attach() now normalizes and validates the new address as well, ensuring consistent behavior and failing fast with a clear error when an invalid address is provided.

@tabaktoni
Copy link
Member

There are use cases when you don't want to attach an address to a contract class. @PhilippeR26 for more details.
Also, what is the point of attaching when you are forced to connect the address in the constructor?

the constructor now always normalizes to lowercase:
This is not a new implementation; it was like that before.

Additionally, here is enforcing a non-empty string for address, which is in conflict with some existing use cases.
For that reason, I would like to reject PR

Copy link
Collaborator

@PhilippeR26 PhilippeR26 left a comment

Choose a reason for hiding this comment

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

In specific cases, myContract.populate() can be used with empty address, when the address is not yet known.
So, it's a false good idea...

Comment on lines +154 to +156
this.address = options.address.toLowerCase();
assert(
typeof this.address === 'string' && this.address.length > 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Besides the mentioned use case that would make the gist of this change undesirable, checking for the string type in the assert doesn't make sense since the code would throw a TypeError before the assert is reached.

@penovicp penovicp closed this Nov 5, 2025
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.

4 participants