Explicit provider ETH addr instead of derived from ETH pub key #336
Conversation
Codecov Report
@@ Coverage Diff @@
## master #336 +/- ##
==========================================
+ Coverage 94.84% 94.85% +<.01%
==========================================
Files 31 31
Lines 2231 2234 +3
==========================================
+ Hits 2116 2119 +3
Misses 115 115 |
|
|
||
| from golem_messages import message | ||
|
|
||
|
|
||
| def pubkey_to_address(pubkey: str) -> str: | ||
| """ | ||
| convert a hex-encoded pubkey into a checksummed address | ||
| convert pubkey into a checksummed address | ||
| pubkey might be hex str or bytes or int |
There was a problem hiding this comment.
No, we don't do that here. pubkey is a string, is typed as such and leave it this way.
There was a problem hiding this comment.
why is that? can you provide some rationale?
There was a problem hiding this comment.
throw-anything-as-an-argument-and-hope-for-the-best just doesn't seem like a maintainable approach. Easier to write and read code when you know the type of the variable.
Also better integration with tools like static analyzer.
There was a problem hiding this comment.
This is how it works in eth lib used by us here. Its a standard there. We support three types of input, not 'anything': string, hexstr and int.
It is usable in such a form but I can introduce dedicated params...
There was a problem hiding this comment.
But we don't need to support three types. At this point almost everywhere we're working on strings. So just provide strings and that's it.
There was a problem hiding this comment.
I needed it to support bytes representation and it seems to me I've found a pretty concise and effective way to achieve it. Much better, I think, that you can observe 5 lines below -- just expand this view and you'll see, that next util function already supports different types on the single input parameter.
This is "The Python Way":
- numpy/scipy prominent example
- pandas here
- Web3 eth API here
One thing you've noticed must be corrected indeed -- the parameter type.
There was a problem hiding this comment.
Python way is also writing unmaintainable and illegible code, doesn't mean we should follow that.
This is the first Union type hint in this repository - meaning, what I said earlier, we don't do that here. We should be consistent within the same project and this case is way too minor to break current conventions.
There was a problem hiding this comment.
I don't approve this change but I have nothing else to add and am dismissing my review since this won't be resolved.
|
|
||
| from golem_messages import message | ||
|
|
||
|
|
||
| def pubkey_to_address(pubkey: str) -> str: | ||
| """ | ||
| convert a hex-encoded pubkey into a checksummed address | ||
| convert pubkey into a checksummed address | ||
| pubkey might be hex str or bytes or int |
There was a problem hiding this comment.
Python way is also writing unmaintainable and illegible code, doesn't mean we should follow that.
This is the first Union type hint in this repository - meaning, what I said earlier, we don't do that here. We should be consistent within the same project and this case is way too minor to break current conventions.
tworec
changed the title
| @@ -16,10 +17,7 @@ class Meta: | |||
| model = dt_tasks.TaskHeader | |||
| exclude = ('requestor_public_key', ) | |||
|
|
|||
| requestor_public_key = factory.LazyFunction( | |||
| lambda: encode_key_id(cryptography.ECCx(None).raw_pubkey) | |||
| ) | |||
There was a problem hiding this comment.
This should still be LazyFunction to get different pubkeys everytime.
| from tests.message import mixins, helpers | ||
| import factory |
There was a problem hiding this comment.
my pylint was complaining, reverting
tests/message/test_concents.py
Outdated
| import math | ||
| import time | ||
| import unittest | ||
| from datetime import datetime, timedelta |
There was a problem hiding this comment.
Now it's not in alphabetical order. I preferred previous ordering.
There was a problem hiding this comment.
made by pycharm import optimizator, reverting
Provider ETH address used explicitly instead of a Provider's public key in WantToComputeTask
Resolves (partly) golemfactory/golem-unlimited#217
complemetary to golemfactory/golem#4170