Implement eth.abc module with abstract base classes for public APIs #1805
Conversation
| in each receipt. Byzantium+, we only need state roots at the end of the block, | ||
| so we *only* call it right before persistance. | ||
|
|
||
| :return: the new state root |
There was a problem hiding this comment.
Anywhere like this where the original abc method had a docstring, the docstring was migrated to the new abc base class.
| @@ -239,8 +139,8 @@ def persist_block(self, | |||
| @classmethod | |||
| def _persist_block( | |||
| cls, | |||
| db: 'BaseDB', | |||
There was a problem hiding this comment.
This resulted in a lot of cleanups like this where we no longer have circular import issues with type hints.
| """ | ||
| Returns an iterable of transactions for the block speficied by the | ||
| given block header. | ||
| """ | ||
| return self._get_block_transactions(header.transaction_root, transaction_class) | ||
|
|
||
| def get_block_transaction_hashes(self, block_header: BlockHeader) -> Iterable[Hash32]: |
There was a problem hiding this comment.
And I found a few places like this where our return types were allowing for things like returning a generator when that is typically a foot-gun.
There was a problem hiding this comment.
👍
BTW, the docstring still says iterable
pipermerriam
force-pushed
the
piper/proper-abc-base-classes
branch
from
0488967 to
8090364
Compare
|
Looks like I need to do some cleanup to get the fixture tests passing. |
pipermerriam
force-pushed
the
piper/proper-abc-base-classes
branch
2 times, most recently
from
c40a72a to
a6c450a
Compare
| """ | ||
| Returns an iterable of transactions for the block speficied by the | ||
| given block header. | ||
| """ | ||
| return self._get_block_transactions(header.transaction_root, transaction_class) | ||
|
|
||
| def get_block_transaction_hashes(self, block_header: BlockHeader) -> Iterable[Hash32]: |
There was a problem hiding this comment.
👍
BTW, the docstring still says iterable
| _error: VMError = None | ||
|
|
||
| # TODO: use a NamedTuple for log entries |
| backend_class = get_db_backend_class(import_path) | ||
| return backend_class(**init_kwargs) | ||
| # mypy doesn't understand the constructor of AtomicDatabaseAPI |
There was a problem hiding this comment.
Does it work if you add back the explicit @abstractmethod with __init__ that was in BaseDB?
@abstractmethod
def __init__(self) -> None:
raise NotImplementedError(
"The `init` method must be implemented by subclasses of BaseDB"
)There was a problem hiding this comment.
Yes, but I don't believe there is a generic __init__ method for AtomicDatabaseAPI implementations. We have one right now that wraps another database but I don't think that's the only constructor.
Also, I'm inclined to remove this functionality entirely because I don't think we actually use it in any meaningful way.
pipermerriam
force-pushed
the
piper/proper-abc-base-classes
branch
from
a6c450a to
42752f0
Compare
What was wrong?
While working with the database components of the library in the trinity codebase I found myself wanting real abstract base class APIs to develop against.
How was it fixed?
Created
eth.abcand created propertABCbase classes for all of the main APIs from the codebase.This should have near zero API changes. The places where APIs changed are minor and have to do with some juggling to define the ABC classes for the
rlp.Serializableobjects.To-Do
Cute Animal Picture