Add ValidateAddress RPCClient to NBitcoin by DanGould · Pull Request #1072 · stratisproject/StratisBitcoinFullNode

DanGould · 2018-01-17T19:39:54Z

tested for bitcoind Mainnet
Return type for ValidateAddress
Sister pull to BreezeProject#25

It seems like RPCClientTests.cs has changed (EstimateFeeRate is gone) What's the best process to incorporate this into that lib?

only returns {isvalid} for P2PKH & P2SH Addresses

…ld/StratisBitcoinFullNode into feature/rpc-verifyaddress

noescape00 · 2018-01-19T08:40:25Z

src/NBitcoin.Tests/RPCClientTests.cs

+        {
+            using (var builder = NodeBuilder.Create())
+            {
+                var node = builder.CreateNode();


108,111,113 - unvar please.

@DanGould Just fix these and we are good to go 👍

dangershony · 2018-01-19T15:21:47Z

Apart for @noescape00 comments looks good to me

fassadlr

Lets look at incorporating JsonResult now 💯

fassadlr · 2018-01-20T12:14:31Z

src/NBitcoin/ValidatedAddress.cs

+
+namespace NBitcoin
+{
+    public class ValidatedAddress


I'm not sure whether we should be adding to NBitcoin like this @DanGould . We are in the process of porting over the library to the full node ;)

I think this is OK, just add [JsonProperty(PropertyName = "isvalid")] on top of the property in order to define the exact format of this json field.

fassadlr · 2018-01-20T12:16:33Z

src/Stratis.Bitcoin.Features.RPC/Controllers/FullNodeController.cs

        [ActionName("validateaddress")]
        [ActionDescription("Returns information about a bech32 or base58 bitcoin address")]
-        public JObject ValidateAddress(string address)
+        public ValidatedAddress ValidateAddress(string address)


@DanGould this is already much better. But lets just return JsonResult here. Then, in the method return the boolean like so:

var isAddressValid = false; return JsonResult(isAddressValid);

There is more to it than just a boolean.
Look here:
https://bitcoin.org/en/developer-reference#validateaddress.
For now that's enough, but I think we can easily add more to it.

@bokobza my apologies, I wasn't aware of the bitcoin developer reference! In that case yes returning ValidateAddress is good 👍

fassadlr · 2018-01-20T12:20:06Z

src/NBitcoin/RPC/RPCClient.cs

+        /// </summary>
+        /// <param name="address">a Bitcoin Address</param>
+        /// <returns>{ IsValid }</returns>
+        public ValidatedAddress ValidateAddress(BitcoinAddress address)


You can really just return a boolean here ;)

Not really. See my other comment.

fassadlr · 2018-01-20T12:21:10Z

src/NBitcoin/RPC/RPCClient.cs

+        public ValidatedAddress ValidateAddress(BitcoinAddress address)
+        {
+            RPCResponse res = SendCommand(RPCOperations.validateaddress, address.ToString());
+            return JsonConvert.DeserializeObject<ValidatedAddress>(res.Result.ToString());


And then you don't have to deserialize the result to a class like this. res.Result should be a boolean... So just cast it?

mikedennis · 2018-01-29T19:28:37Z

There have been fairly recent changes to NBitcoin with RPC fee rate tests. There now seem to be some estimatesmartfee tests. I don't believe I migrated those across as I think they target a newer version of bitcoin RPC. See MetacoSA/NBitcoin@0d683ce#diff-280b17ef00043f2fb2b29ad882a40452

bokobza · 2018-01-31T15:55:24Z

@DanGould Are you able to work on this PR? If not, please let us know. Thank you 😃

DanGould · 2018-01-31T16:38:59Z

@bokobza I will have time Thursday & Friday. Classes just started and I've just about found the groove.

fassadlr · 2018-02-01T11:17:50Z

@DanGould please check the comments from @noescape00 and once that is fixed we can merge this change 👍

fassadlr · 2018-02-01T11:25:40Z

src/NBitcoin.Tests/RPCClientTests.cs

+        {
+            using (var builder = NodeBuilder.Create())
+            {
+                var node = builder.CreateNode();


@DanGould Just fix these and we are good to go 👍

bokobza · 2018-02-02T01:58:28Z

@fassadlr I'd like to see the JsonProperty in ValidatedAddress.cs as well, please. Before you merge this.

fassadlr · 2018-02-02T09:47:53Z

@bokobza will do 💯

fassadlr

A couple of small changes

fassadlr · 2018-02-03T12:17:28Z

src/Stratis.Bitcoin.Features.RPC/Controllers/FullNodeController.cs

-            var res = new JObject();
-            res["isvalid"] = false;
+            var res = new ValidatedAddress();
+            res.IsValid = false;


This is implied if the bool is not specifically set to false ;) so you can remove this line 👍

fassadlr · 2018-02-03T12:20:17Z

src/Stratis.Bitcoin.Features.RPC/Controllers/FullNodeController.cs

        /// </summary>
        /// <param name="address">bech32 or base58 BitcoinAddress to validate.</param>
-        /// <returns>JObject containing a boolean indicating address validity</returns>
+        /// <returns>ValidatedAddress containing a boolean indicating address validity</returns>


Change this to:

/// <returns><see cref="ValidatedAddress"/> containing a boolean indicating address validity.</returns>

bokobza · 2018-02-05T09:34:54Z

@fassadlr do you mind if we push this as it is? @DanGould doesn't have much time between his classes.
Worst case we can do a quick follow-up PR.

fassadlr · 2018-02-05T09:43:33Z

@bokobza OK sweet 👍

* simple implement validateaddresses only returns {isvalid} for P2PKH & P2SH Addresses * support verify bech32 addresses * delete innacurate comment * fix invalid method doc markup * fassadlr's clean code recommendations * validateaddress only for passed expectedNetwork * remove redundant FormatExceptions * network to expectedNetwork so builds * make ValidatedAddress return type * add VerifyAddress to NBitcoin RPCClient * unvar ValidateAddress RPCTest * define ValidatedAddress JsonProperty isvalid

DanGould added 12 commits January 10, 2018 15:47

simple implement validateaddresses

9fc7cee

only returns {isvalid} for P2PKH & P2SH Addresses

support verify bech32 addresses

9c3fd15

Merge branch 'master' into feature/rpc-verifyaddress

9846cdf

delete innacurate comment

16b5c16

Merge branch 'feature/rpc-verifyaddress' of https://github.com/DanGou…

c0d515c

…ld/StratisBitcoinFullNode into feature/rpc-verifyaddress

fix invalid method doc markup

7f6637e

fassadlr's clean code recommendations

3590ffc

validateaddress only for passed expectedNetwork

738be12

remove redundant FormatExceptions

f85906f

network to expectedNetwork so builds

213edd1

make ValidatedAddress return type

a05c562

add VerifyAddress to NBitcoin RPCClient

260264d

DanGould mentioned this pull request Jan 17, 2018

fix/rpc-validateaddress BreezeHub/BreezeProject#25

Open

merge master

1b9f2a6

noescape00 reviewed Jan 19, 2018

View reviewed changes

fassadlr suggested changes Jan 20, 2018

View reviewed changes

fassadlr approved these changes Feb 1, 2018

View reviewed changes

fassadlr suggested changes Feb 1, 2018

View reviewed changes

DanGould added 2 commits February 2, 2018 13:24

unvar ValidateAddress RPCTest

b8d3e0b

define ValidatedAddress JsonProperty isvalid

1bee717

DanGould force-pushed the feature/rpc-verifyaddress branch 2 times, most recently from 0836455 to e0cfe2d Compare February 2, 2018 22:52

Merge branch 'master' into feature/rpc-verifyaddress

a0f4bdb

DanGould force-pushed the feature/rpc-verifyaddress branch from e0cfe2d to a0f4bdb Compare February 2, 2018 23:13

fassadlr suggested changes Feb 3, 2018

View reviewed changes

fassadlr approved these changes Feb 5, 2018

View reviewed changes

fassadlr merged commit 5740871 into stratisproject:master Feb 5, 2018

Conversation

DanGould commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dangershony commented Jan 19, 2018

Uh oh!

fassadlr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikedennis commented Jan 29, 2018

Uh oh!

bokobza commented Jan 31, 2018

Uh oh!

DanGould commented Jan 31, 2018

Uh oh!

fassadlr commented Feb 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bokobza commented Feb 2, 2018

Uh oh!

fassadlr commented Feb 2, 2018

Uh oh!

fassadlr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bokobza commented Feb 5, 2018

Uh oh!

fassadlr commented Feb 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DanGould commented Jan 17, 2018 •

edited

Loading