-
Notifications
You must be signed in to change notification settings - Fork 20
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
fix/rpc-validateaddress #25
base: master
Are you sure you want to change the base?
Conversation
Great, I think you should probably continue working on this in a separate branch for the time being. Are the build errors due to the required changes you refer to? |
bitcoinNetwork = "StratisTest"; | ||
else if (TumblerNetwork == Network.StratisRegTest) | ||
bitcoinNetwork = "StratisRegTest"; | ||
else if (TumblerNetwork == Network.Main) |
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.
Are we able to get the network name as a string in a cleaner way than checking against every type?
@@ -83,7 +83,7 @@ public RPCClient ConfigureRPCClient(Network network) | |||
try | |||
{ | |||
var address = new Key().PubKey.GetAddress(network); | |||
var isValid = ((JObject)rpcClient.SendCommand("validateaddress", address.ToString()).Result)["isvalid"].Value<bool>(); | |||
var isValid = rpcClient.ValidateAddress(address).IsValid; |
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.
@DanGould if you fix the RPCClient on the FullNode side, this will be:
var isValid = rpcClient.ValidateAddress(address);
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.
@DanGould but the code here is already MUCH cleaner. Good job 👍
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.
Small comment re FullNode changes.
if (configFile.GetOrDefault<string>("network", "testnet").Equals("stratismain")) | ||
{ | ||
TumblerNetwork = Network.StratisMain; | ||
} |
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.
Shouldn't this be converted from 3 if
s to if->else if -> else if
or for a cleaner code to a switch
statement. There is no need to test all conditions if one of the succeeds
else if (breezeConfig.TumblerNetwork == Network.StratisTest) | ||
argsTemp.Add("-stratistest"); | ||
else if (breezeConfig.TumblerNetwork == Network.StratisRegTest) | ||
argsTemp.Add("-stratisregtest"); |
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.
Feels like we need some sort of extension method for a Network enum so that we can put that logic into one place and replace all this code with just argsTemp.Add(breezeConfig.TumblerNetwork.ToSuffix())
or something along those lines. Same for bitcoin network string above. Something like bitcoinNetwork = TumblerNetwork.ToNamedString()
would be cleaner
ValidateAddress