Wrapper for a transfer transaction API

Question

I'm looking for any kind of advice, like when or where to do exception handling, the overall library structure/layout, usage of classes/partial classes, code efficiency, naming conventions, and so on and so forth.

Note: Knowledge of cryptocurrency technology would be an advantage.

The following code is the section that covers this API call. The whole wrapper can be found here. I will post code for one specific operation but would love if someone would be willing to do a full review.

Connecting to NIS:

using System;


namespace NemApi
{
public static class Connection
{
    public static string partialUri { get; set; }

    static Connection()
    {
        NisConnect();
    }

    public static void NisConnect(String nodeIP, int port)
    {
        partialUri = String.Format("http://{0}:{1}", nodeIP, port);
    }

    public static void NisConnect()
    {
        partialUri = String.Format("http://{0}:{1}", "127.0.0.1", 7890);
    }
}
}

When making any request, I need to work with JSON. For example, when sending a transaction, I populate a JSON data transfer object and then serialize it into a JSON string:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace NemApi.DTOs.AccountTransactionObjects
{
public class TransactionVersionOne
{
    public class Message
    {
        public string payload { get; set; }
        public int type { get; set; }
    }

    public class Transaction
    {
        public long timeStamp { get; set; }
        public long amount { get; set; }
        public long fee { get; set; }
        public string recipient { get; set; }
        public int type { get; set; }
        public long deadline { get; set; }
        public Message message { get; set; }
        public int version { get; set; }
        public string signer { get; set; }
    }

    public class RootObject
    {
        public Transaction transaction { get; set; }
        public string privateKey { get; set; }
    }
}
}

Populating the DTO and post the request:

using NemApi;
using NemApi.DTOs.AccountTransactionObjects;
using NemApi.DTOs.NisStatusObjects;
using Newtonsoft.Json;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Net.Http;
using System.Text;
using System.Threading.Tasks;

namespace NemApi
{
public partial class PostTransaction
{
    static HttpClient client = new HttpClient();

    public static async Task<GetPrepareAnnounce.Response> VersionOne(
                string privKey,
                long amount,
                long fee,
                int deadline,
                string recipient,
                string signer,
                string messagePayload = "",
                bool encrypted = false)
    {

        Nis.NetworkTime time = await GetNis.NetworkTime();

        TransactionVersionOne.Transaction Tx = new TransactionVersionOne.Transaction();
        Tx.amount = 10000000;
        Tx.fee = fee * 1000000;
        Tx.deadline = (time.receiveTimeStamp / 1000) + deadline;
        Tx.recipient = recipient;
        Tx.signer = signer;
        Tx.timeStamp = time.receiveTimeStamp / 1000;
        Tx.type = 257;
        Tx.version = 1744830465;

        TransactionVersionOne.Message Msg = new TransactionVersionOne.Message();
        Msg.payload = messagePayload;
        Msg.type = encrypted ? 2 : 1;
        Tx.message = Msg;

        TransactionVersionOne.RootObject RPA = new TransactionVersionOne.RootObject();
        RPA.privateKey = privKey;
        RPA.transaction = Tx;

        string uri = String.Concat(Connection.partialUri, "/transaction/prepare-announce");

        var response = await client.PostAsync(uri, new StringContent(JsonConvert.SerializeObject(RPA).ToString(), Encoding.UTF8, "application/json"));

        var content = response.Content;
        {
            var json = await content.ReadAsStringAsync();

            var model = JsonConvert.DeserializeObject<GetPrepareAnnounce.Response>(json);

            return model;
        }
    }
}
}

This function is then called as follows:

    public static async void SendTransactionV1()
    {
        GetPrepareAnnounce.Response a = await PostTransaction.VersionOne(
            "<privat key>",          //priv key
            1000000,                 // amount
            10,                      // fee
            1000,                    // deadline
            "<address>",             // recipient
            "<sender public key>"    // sender pubkey
        );
        Console.WriteLine(a.message);
    }

Answer 1

Bigger Issues

Making Connection static is usually asking for trouble. The reason is you can't swap out the connection for something else, you can't have it run it with two different servers in the same process.

Static classes also make it very difficult to unit test code.

I recommend you make a IConnection interface that describes your Connection. Then, create a single connection and pass it down through your various constructors. It is annoying, but if it really bothers you, you could consider using an IoC container such as Ninject to handle the constructor chains for you.

I highly recommend you use enums or symbolic types for some of your constants.

Tx.type = 257; Msg.type = encrypted ? 2 : 1;

Having numbers like that are asking for trouble. Newton JSON has a fair amount of work in handling that so you can use C#-isms and reduce your chance of failures.

I recommend deadline is a DateTimeOffset or some sort of time-based element that you convert inside your DTO class. That will reduce the chance of errors but also make it more obvious how it works. As it stands know, you need to know a lot more of the internals to figure out how to format that deadline properly. If it is an offset, use TimeSpan instead. Things like the / 1000 should be encapsulated inside the class, not in the consumers.

While your sender and recipient are strings, it would reduce the chance of errors if you wrap that in a NisPublicKey (NisPrivateKey for sendiner?) or some sort of class that indicates its purpose. Again, it will make it easier to detect errors.

Fee processing should be encapsulated. Your DTO can hold the fee and calculate it (or have a wrapper that does it).

VersionOne as a method name doesn't really scale well. I highly recommend you make your class based on one (and use numbers instead of text).

This code look wrong. The brace doesn't mean anything and gives the impression it is under the response.Content which it is not.

    var content = response.Content;
    {
        var json = await content.ReadAsStringAsync();

        var model = JsonConvert.DeserializeObject<GetPrepareAnnounce.Response>(json);

        return model;
    }

Minor

You might consider using UriBuilder and Uri instead of strings. I know it is a minor thing, but the compiler will catch the three in the morning mistakes if you have type safety.

Cosmetic

If you are following Microsoft's suggested conventions, all of your properties and methods should be PascalCase. So, partialUri would be PartialUri.

Generally speaking, I would have named Connection to NisConnection and then make the method names simpler (Connect).

Consider avoiding generic class names (Message) since Intellisense will create a lot of noise since multiple packages with that name will show up.

I like one class per file, but I find it easier to work with.

NewtonJSON has the ability to convert PascalCase properties into camelCase automatically. That will let you use the conventions of your language without making things feel "off".

If you use Microsoft's conventions, variables are camelCase. So Tx would be tx (though, I would have used transaction instead).

Answer 2

Avoid redundant method implementations

public static void NisConnect(String nodeIP, int port)
{
     partialUri = String.Format("http://{0}:{1}", nodeIP, port);
}

public static void NisConnect()
{
     partialUri = String.Format("http://{0}:{1}", "127.0.0.1", 7890);
}

Have one overload perform the setter, and have the other call that one.

public static void NisConnect()
{
     NisConnect("127.0.0.1", 7890);
}

Use meaningful class names

public class RootObject  // <- meaningless name
{
    public Transaction transaction { get; set; }
    public string privateKey { get; set; }
}

Avoid verbose variable declarations

Use var when the instatiated type is the same as the declared.

// TransactionVersionOne.Transaction Tx = new TransactionVersionOne.Transaction();
var Tx = new TransactionVersionOne.Transaction();

Pascal-case variable names

Also use a meaningful variable name.

 // var Tx = new TransactionVersionOne.Transaction();
 var transaction = new TransactionVersionOne.Transaction();

Title-case property names

// public static string partialUri { get; set; }
public static string PartialUri { get; set; }

Use property initializers

TransactionVersionOne.Message Msg = new TransactionVersionOne.Message();
Msg.payload = messagePayload;
Msg.type = encrypted ? 2 : 1;
Tx.message = Msg;

var transactionMessage = new TransactionVersionOne.Message() 
{
    Payload = messagePayload,
    Type = encrypted ? 2 : 1,
    Message = message;
};

Don't use redundant code blocks

var content = response.Content;
{
      var json = await content.ReadAsStringAsync();
      var model = JsonConvert.DeserializeObject<GetPrepareAnnounce.Response>(json);
      return model;
}

var content = response.Content;
var json = await content.ReadAsStringAsync();
var model = JsonConvert.DeserializeObject<GetPrepareAnnounce.Response>(json);
return model;

Define the async-await pattern correctly

public static async void SendTransactionV1() // ..

public static async Task SendTransactionV1() // ..