Page MenuHomePhabricator

[electrum] fix all mypy errors in address.py
Changes PlannedPublic

Authored by PiRK on Nov 17 2023, 10:15.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Summary

Notes:

  • the OpCodes enum was not complete, all integers in range 1--75 are also valid push opcodes. So I had to either change the return type for Script.get_ops to int instead of OpCodes, or else cast all integers to OpCodes and vice-versa (when the code also represents a length). I chose the second solution, which is more correct, which requires making the OpCodes enum complete.
  • dynamically patching the Address instance with _addr2str_cache in Address.__new__ cannot be support by mypy. So I've added an __init__ method to properly initialize this parameter.
Test Plan

mypy --ignore-missing-imports electrumabc/address.py | grep "^electrumabc/address.py"

python test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
mypy
Lint
Lint Errors
SeverityLocationCodeMessage
Errorelectrum/electrumabc/address.py:32F401flake8 F401
Unit
No Test Coverage
Build Status
Buildable 25655
Build 50890: Build Diffelectrum-tests
Build 50889: arc lint + arc unit

Event Timeline

PiRK requested review of this revision.Nov 17 2023, 10:15
Fabien requested changes to this revision.Nov 17 2023, 10:41
Fabien added a subscriber: Fabien.
Fabien added inline comments.
electrum/electrumabc/bitcoin.py
170 ↗(On Diff #43142)

Nice ascii to decimal conversion table

electrum/electrumabc/networks.py
61 ↗(On Diff #43142)

The changes to this file seem unrelated. Also what does this do ? Declare a value that is unset ?

This revision now requires changes to proceed.Nov 17 2023, 10:41
electrum/electrumabc/bitcoin.py
170 ↗(On Diff #43142)

I tried various dynamic ways of setting this, but mypy can't digest the dynamic ways.

For instance :

class BaseOpCodes(IntEnum):
    ...

OpCodes = IntEnum(
    "OpCodes",
     dict(
        {op.name: op.value for op in BaseOpCodes}, 
        **{f"OP_PUSH_{n}_BYTES": n for n in range(1, BaseOpCodes.OP_PUSHDATA1)}
    )
)
electrum/electrumabc/networks.py
61 ↗(On Diff #43142)

These attributes happen to be declared and set in every child class, but mypy has no way of knowing this and cannot rely on it. Adding them to the abstract parent class hints to mypy and various IDEs that the child classes should set them.

mypy raises two errors in address.py because some variables are anotated with AbstractNet:

electrumabc/address.py:507: error: "AbstractNet" has no attribute "ADDRTYPE_P2PKH"  [attr-defined]
electrumabc/address.py:509: error: "AbstractNet" has no attribute "ADDRTYPE_P2SH"  [attr-defined]

But I should really change it to

class AbstractNet(ABC):
     ...
    ADDRTYPE_P2PKH: int = NotImplemented
    ADDRTYPE_P2SH: int = NotImplemented
PiRK planned changes to this revision.Nov 17 2023, 14:34

I'm still thinking about the best strategy to enable mypy on that codebase. It would be great to have it, it could save us from obvious bugs such as D14795. On the other hand, there is a very long way to go until all existing errors and warnings are fixed. So either we activate it file by file, the same way as I managed to activate flake8 back in the days (see for instance https://github.com/Bitcoin-ABC/ElectrumABC/pull/161/commits/d512bd4ef2d41e2ab807cddc21feab18f1f120cb), or we make a clever script that makes a list of existing errors and tests if no new error is introduced. But that could prove very noisy whenever some dependency changes a typehint in a function we are calling.

In D14808#332663, @PiRK wrote:

I'm still thinking about the best strategy to enable mypy on that codebase. It would be great to have it, it could save us from obvious bugs such as D14795. On the other hand, there is a very long way to go until all existing errors and warnings are fixed. So either we activate it file by file, the same way as I managed to activate flake8 back in the days (see for instance https://github.com/Bitcoin-ABC/ElectrumABC/pull/161/commits/d512bd4ef2d41e2ab807cddc21feab18f1f120cb), or we make a clever script that makes a list of existing errors and tests if no new error is introduced. But that could prove very noisy whenever some dependency changes a typehint in a function we are calling.

The second option, if possible is better because it prevents you from introducing new issues.