-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Explicit provider ETH addr instead of derived from ETH pub key #336
Changes from 10 commits
009a0d0
42c23ab
dbf8606
b3c8937
8d01a04
aa78f3e
093920f
8a70667
d1070e9
70e0416
fb97112
089f077
27add20
1cec8a1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,17 +2,18 @@ | |
import copy | ||
from uuid import UUID | ||
|
||
from eth_utils import to_checksum_address, keccak | ||
from eth_utils import to_checksum_address, keccak, hexstr_if_str | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, we don't do that here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is that? can you provide some rationale? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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":
One thing you've noticed must be corrected indeed -- the parameter type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Python way is also writing unmaintainable and illegible code, doesn't mean we should follow that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't approve this change but I have nothing else to add and am dismissing my review since this won't be resolved. |
||
""" | ||
return to_checksum_address( | ||
keccak(decode_hex(pubkey))[12:].hex(), | ||
hexstr_if_str(keccak, pubkey)[12:].hex(), | ||
) | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import unittest | ||
|
||
from eth_utils import is_checksum_address | ||
|
||
from golem_messages.factories.helpers import random_eth_address | ||
|
||
|
||
class HelpersTest(unittest.TestCase): | ||
|
||
def test_random_eth_address(self): | ||
addr = random_eth_address() | ||
self.assertTrue(is_checksum_address(addr)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import unittest | ||
|
||
from golem_messages.factories.tasks import WantToComputeTaskFactory | ||
from golem_messages.utils import pubkey_to_address | ||
|
||
|
||
class HelpersTest(unittest.TestCase): | ||
|
||
# provider_ethereum_address is not bound to provider_public_key | ||
# below binding is only for compatibility with Concent tests | ||
def test_WTCT_factory_pubkey_bound_to_addr(self): | ||
wtct = WantToComputeTaskFactory() | ||
self.assertEqual(wtct.provider_ethereum_address, | ||
pubkey_to_address(wtct.provider_public_key)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,19 @@ | ||
# pylint: disable=protected-access | ||
from datetime import datetime, timedelta | ||
import math | ||
import time | ||
import unittest | ||
from datetime import datetime, timedelta | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now it's not in alphabetical order. I preferred previous ordering. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. made by pycharm import optimizator, reverting |
||
|
||
import freezegun | ||
|
||
from golem_messages import cryptography | ||
from golem_messages.datastructures import promissory | ||
from golem_messages import exceptions | ||
from golem_messages import message | ||
|
||
from golem_messages import factories | ||
from golem_messages import message | ||
from golem_messages import shortcuts | ||
from golem_messages.datastructures import promissory | ||
from golem_messages.message import concents | ||
from golem_messages.utils import encode_hex as encode_key_id | ||
|
||
from golem_messages.utils import pubkey_to_address | ||
from tests.message import helpers | ||
from tests.message import mixins | ||
|
||
|
@@ -142,7 +140,9 @@ def test_subtask_id(self): | |
def test_concent_promissory_note(self): | ||
provider_keys = cryptography.ECCx(None) | ||
wtct = factories.tasks.WantToComputeTaskFactory( | ||
provider_public_key=encode_key_id(provider_keys.raw_pubkey) | ||
provider_ethereum_address=pubkey_to_address( | ||
provider_keys.raw_pubkey | ||
) | ||
) | ||
srv: concents.SubtaskResultsVerify = self.FACTORY(**{ | ||
'subtask_results_rejected__' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should still be
LazyFunction
to get different pubkeys everytime.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done