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

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

Severity	Location	Code	Message
Error	electrum/electrumabc/address.py:32	F401	flake8 F401

Unit

No Test Coverage

Build Status

Buildable 25655
Build 50890: Build Diff	electrum-tests
Build 50889: arc lint + arc unit

Event Timeline

PiRK created this revision.Nov 17 2023, 10:15

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 17 2023, 10:15

PiRK requested review of this revision.Nov 17 2023, 10:15

Harbormaster completed remote builds in B25655: Diff 43141.Nov 17 2023, 10:15

remove unused Dict

Harbormaster completed remote builds in B25656: Diff 43142.Nov 17 2023, 10:22

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

PiRK added inline comments.Nov 17 2023, 12: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

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.