Skip to content
This repository has been archived by the owner on Nov 15, 2021. It is now read-only.

add validate address rpc call + tests #198

Merged
merged 4 commits into from
Jan 27, 2018
Merged

add validate address rpc call + tests #198

merged 4 commits into from
Jan 27, 2018

Conversation

ixje
Copy link
Member

@ixje ixje commented Jan 25, 2018

What current issue(s) does this address?, or what feature is it adding?
add validate address rpc call + tests

address part of #189

How did you solve this problem?
implement call + tests

How did you make sure your solution works?
tests

Did you add any tests?
yes

Are there any special changes in the code that we should be aware of?
no

Did you run make lint and make test?
yes, no

Are you making a PR to a feature branch or development rather than master?
feature

@coveralls
Copy link

coveralls commented Jan 25, 2018

Coverage Status

Coverage increased (+0.05%) to 72.673% when pulling 0380bd0 on ixje:rpc_validateaddress into 0cc8e16 on CityOfZion:jsonrpc1.

@@ -128,6 +129,21 @@ def parse_uint_str(self, param):
return param[2:]
return param

def validateaddress(self, params):
# check for [] parameter or [""]
if params is None or params[0] == '':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code doesn't yet check for an empty list (it is not None). perhaps upgrade to if params is None or len(params) == 0 or params[0] == '':

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have anticipated this as I initially was surprised as well. However, this code is correct given the current gen_rpc_req implementation. Here's why

req = self._gen_rpc_req("validateaddress", params=[])

returns a dict without the params keyword, therefor if params is None actually tests for the empty list. I'm assuming an actual request will be parsed similarly.

In [3]: def test_list(params=None):
   ...:     if params:
   ...:         print("yes params")
   ...:     else:
   ...:         print("no params")
   ...:

In [4]: test_list([])
no params

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what you are saying. but this is not 100% correct. if not params matches an empty list, if params is None doesn't. you can just replace this if here with if not params or params[0] == '': for the intended effect.

@@ -237,8 +253,7 @@ def json_rpc_method_handler(self, method, params):
raise NotImplementedError()

elif method == "validateaddress":
raise NotImplementedError()

return self.validateaddress(params)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one line whitespace before the next elif would be nice

@metachris
Copy link
Contributor

very nice 👍
just 2 minor comments

@localhuman
Copy link
Collaborator

The way neon-js and c# checks valid address is to convert to scripthash and then back. if it comes back the same it is valid

@ixje
Copy link
Member Author

ixje commented Jan 26, 2018

@localhuman yeah I'm aware there are much better ways to do it, but I just followed the RpcServer.cs code from here to here. The parts you talk about is in the actual Wallet code (I do the same in my mobile wallet)

@ixje
Copy link
Member Author

ixje commented Jan 27, 2018

@localhuman I'd like to keep the current validation test until we've implemented isValidPublicAddress discussed here in neo-python-core. That can then be the extensive version that also does the checksum validation you mentioned. Would that work for you?

@localhuman
Copy link
Collaborator

That works for me. Would you mind fixing conflicts :)

@localhuman
Copy link
Collaborator

best commit message ever 🤣

@localhuman localhuman merged commit 6df3be1 into CityOfZion:jsonrpc1 Jan 27, 2018
@ixje ixje deleted the rpc_validateaddress branch March 11, 2018 09:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants