SeedSigner / seedsigner

Use an air-gapped Raspberry Pi Zero to sign for Bitcoin transactions! (and do other cool stuff)
MIT License
720 stars 168 forks source link

Fix issue caused by having is_sign_message method and property of same name #578

Closed BamaHodl closed 3 months ago

BamaHodl commented 4 months ago

Description

Small fix of an issue caused by having a method and property of the same name. In scan_views.py, the check: elif self.decoder.is_sign_message: always passes the check because self.decoder.is_sign_message is of type method, evaluates to True because it's not None. It doesn't seem to be causing any issues currently because it's the final elif check for the QR type, however when adding new QR types below that elif, it will never flow to them. I experienced this when prototyping a new QR functionality. The following example code demonstrates the issue:

class TestClass:
    @property
    def test_it(self):
        return False

    def test_it(variable : str):
        return False

t = TestClass()
if not t.test_it:
    print ("Everything fine, got False for type " + str(type(t.test_it)))
else:
    print ("Not fine, wanted the property function but rather is of type " + str(type(t.test_it)))

Which yields: Not fine, wanted the property function but rather is of type <class 'method'>

This pull request is categorized as a:

Checklist

If you modified or added functionality/workflow, did you add new unit tests?

I have tested this PR on the following platforms/os:

dbast commented 4 months ago

+1 nice finding... pyflakes (passive Python code checker) confirms the finding:

src/seedsigner/models/decode_qr.py:510:9: F811 Redefinition of unused `is_sign_message` from line 290
    |
509 |     @staticmethod
510 |     def is_sign_message(s):
    |         ^^^^^^^^^^^^^^^ F811
511 |         return type(s) == str and s.startswith("signmessage")
    |
    = help: Remove definition: `is_sign_message`
newtonick commented 3 months ago

ACK