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

Add utility checking functions for determining if a type is known. #125

Closed
pipermerriam opened this issue Mar 21, 2019 · 9 comments
Closed

Comments

@pipermerriam
Copy link
Member

Inspired by: ethereum/eth-account#57 (comment)

What was wrong?

The is_encodable function can raise an exception if the type is unknown. This is probably un-avoidable, but it does leave 3rd party libraries in an awkward position when they might be dealing with an unknown or invalid type string.

How can it be fixed?

Not sure which of these is the right API but adding one or both of the following utilities.

  1. registry.is_valid_type(type_str) -> return True/False (never raises exception)
  2. registry.validate_type(type_str) -> returns None or raises eth_utils.ValidationError
@carver
Copy link
Collaborator

carver commented Mar 23, 2019

👍 for adding both

@davesque
Copy link
Contributor

So that would just be something like this?

def is_valid_type(self, typ):
    if coder_found_for_type:
        return True
    else:
        return False

def validate_type(self, typ):
    if coder_found_for_type:
        return
    else:
        raise ValidationError(...)

@pipermerriam
Copy link
Member Author

Yeah, those implementations seem on-par with what I was thinking. Worth evaluating whether those would suffice for the linked eth-account issue since that was the primary motivation for this.

@davesque
Copy link
Contributor

I was looking into implementing a has_encoder method on the ABIRegistry class just now. That would return True or False depending on if a given type string has an encoder in the registry. However, it's making me realize that the granularity of errors throw by the PredicateMapping class isn't great. Ideally, I would have a PredicateMappingError class and at least two subclasses of that for the purposes of that new method. Those would be something like NoEntriesFound and MultipleEntriesFound. Then I could catch the NoEntriesFound error specifically in the has_encoder method. I know that you two have voiced a preference for using built-in error types but, in this case, I don't think that's going to work terribly well. I'll just go ahead and proceed with custom error types unless I hear any strong objections to that plan.

@davesque
Copy link
Contributor

Or, we could subclass ValueError to keep things backwards compatible but that just feels weird.

@pipermerriam
Copy link
Member Author

pipermerriam commented Mar 25, 2019

You can do the subclass approach with the plan to break that link in the next major version bump. In general I'm a fan of custom exceptions for things that a user would want to catch and only using things like ValueError or TypeError for things that are true programmer mistakes and not things that they should encounter during runtime (and thus that they shouldn't need to differentiate between exception types)

@davesque
Copy link
Contributor

Just made PR #128 to fix this.

@fubuloubu
Copy link
Contributor

#128 was merged, close this?

Note, I tracked this down via the following comment:
https://github.com/ethereum/eth-account/blob/a1e5ab2400ae6ed70d91a1226ec3643d27b29e56/eth_account/_utils/structured_data/hashing.py#L100

@davesque
Copy link
Contributor

@fubuloubu Ahh, yeah. Thanks!

pacrob pushed a commit to pacrob/eth-abi that referenced this issue Mar 25, 2024
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

No branches or pull requests

4 participants