Skip to content
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

Refactor for duplicate code in contract internal implementations #3579

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

darwintree
Copy link

@darwintree darwintree commented Jan 16, 2025

What was wrong?

Closes #3561

In internal contract implementations, there might be duplicate implementations repeating same code for async and non-async functions, for example, ContractFunctions and AsyncContractFunctions.

How was it fixed?

This case is because of:

  • hardcoded class name
  • type hint requirements

As for previous one, they can be resolved using self.class to other techiniques to dynamically get the right class.
As for latter one, Self combined with Generic would help solve the issue

Todo:

  • Clean up commit history
  • Add or update documentation related to these changes
  • Add entry to the release notes

Cute Animal Picture

image

@darwintree darwintree marked this pull request as ready for review January 16, 2025 07:24
@darwintree darwintree changed the title Improve contract Remove duplicate code for contract internal implementations Jan 16, 2025
@darwintree darwintree changed the title Remove duplicate code for contract internal implementations Refactor for duplicate code in contract internal implementations Jan 16, 2025
Copy link
Contributor

@reedsa reedsa left a comment

Choose a reason for hiding this comment

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

These updates are looking great, thanks for your contributions. I've mentioned a few changes to additional cleanup and suggested exception messages. Should be good to go after that!

]

# Check that arguments in call match a function ABI
num_attempts = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe these num_attempts variables can be removed. They were mistakenly left in from a previous change.

@@ -785,6 +938,46 @@ def __hasattr__(self, function_name: str) -> bool:
except ABIFunctionNotFound:
return False

def __iter__(self) -> Iterable[TContractFn]:
Copy link
Contributor

Choose a reason for hiding this comment

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

The corresponding function __iter__ in ContractFunction still exists and should be removed.

for func in self._functions:
yield self[abi_to_signature(func)]

def __getattr__(self, function_name: str) -> TContractFn:
Copy link
Contributor

Choose a reason for hiding this comment

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

The corresponding function __getattr__ in ContractFunction still exists and should be removed.

function_name,
)

def __getitem__(self, function_name: str) -> TContractFn:
Copy link
Contributor

Choose a reason for hiding this comment

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

The corresponding function __getitem__ in ContractFunction still exists and should be removed.

web3/contract/base_contract.py Outdated Show resolved Hide resolved
ccip_read_enabled: Optional[bool] = None,
) -> Any:
"""
Should be implemented by child class.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Should be implemented by child class.
Implementation of ``call`` should create a callable contract function and execute it using the `eth_call` interface.

@@ -122,7 +106,7 @@ class AsyncContractEvent(BaseContractEvent):
# mypy types
w3: "AsyncWeb3"

def __call__(self, *args: Any, **kwargs: Any) -> "AsyncContractEvent":
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm inclined to leave this as is since the returned object is a new copy of the event.

Suggested change
def __call__(self, *args: Any, **kwargs: Any) -> "AsyncContractEvent":
def __call__(self, *args: Any, **kwargs: Any) -> "AsyncContractEvent":

Copy link
Author

Choose a reason for hiding this comment

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

I think they are nearly the same and Self can keep consistency. Self is used to indicate type hints, but did not imply if it is self or a copy of it

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine with me. Should this __call__ and the one in the ContractEvent class move to base_contract? If not, I noticed the ContractEvent.__call__ has the type "ContractEvent" rather than self, probably best to make them consistent.

Copy link
Author

Choose a reason for hiding this comment

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

Done. Moved into BaseContractEvent

web3/contract/base_contract.py Show resolved Hide resolved
newsfragments/3579.internal.rst Outdated Show resolved Hide resolved
@reedsa reedsa requested review from fselmo and kclowes January 21, 2025 20:23
@darwintree
Copy link
Author

@reedsa Thanks for reviewing.

Changes already made except for AsyncContractEvent.__call__. I think the mentioned problem is not an issue and it is ok to keep stay unchanged or using Self for consistent type hint. Would know your idea.

contract_function = None
for abi in function_abis_with_arg_count:
try:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need pass here.

@darwintree
Copy link
Author

@reedsa I think the above mentioned issues are resolved now

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.

DRY up common code in contract classes
2 participants