suggestion for address type #11
Conversation
|
I definitely like where your heads at. This is my main motivation for Issue #5. |
| return (_bytes[0] >> 4) switch | ||
| { | ||
| 0x01 => AddressType.Base, //@TODO: derive all AddressTypes | ||
| 0x06 => AddressType.Enterprise, |
There was a problem hiding this comment.
I believe Reward type is the same as Enterprise. I haven't done a lot of testing around this, but thats what i came up with from reading the Rust code
There was a problem hiding this comment.
after reviewing the AddressService, i believe im wrong
| private byte[] _bytes; | ||
| private string _address; |
There was a problem hiding this comment.
There have been times when having access to both of would have been very useful.
There was a problem hiding this comment.
ToString() gives you the Address
GetBytes() gives you the Bytes
the rationale for using methods instead of properties is that properties don't support formatting or endianness.... maybe that is a feature for the future...
There was a problem hiding this comment.
One thing that we need to do is make a string encoder so people can query by hash. https://github.com/CardanoSharp/cardanosharp-demo/blob/CreateTransactionExplorer/src/Infastructure/Persistence/CardanoEFCore/Queries.cs If you look here at line 70, I did a raw SQL query from the passed in hash. The Postgres hash type is a 32 byte array I believe, and when trying to convert a string to the proper Postgres type it was a pain that I couldn't get. So I think it would be super useful to have an extenstion method that could turn strings into that postgres types.
There was a problem hiding this comment.
Ill dive into how the tx id is created. I know the cardano cli can tell you want it will be before you even submit so im sure we can figure it out. Im sure its something along the lines of byte[] -> blake hash
|
I feel like a type like this could replace the |
In my opinion I suggest a repurposing of services for use in DI containers. interface IAddressService : IDisposable
{
// default Network
NetworkType NetworkType { get; }
// legacy support
string CreateAddress(byte[] payment, byte[] stake, AddressType addressType);
string CreateAddress(byte[] payment, byte[] stake, NetworkType networkType, AddressType addressType);
// new methods
AddressBase CreateAddress(RootKey rootKey, AddressType addressType);
AddressBase CreateAddress(RootKey rootKey, AddressType addressType, NetworkType networkType);
PaymentAddress CreatePaymentAddress(RootKey rootKey);
PaymentAddress CreatePaymentAddress(RootKey rootKey, NetworkType networkType);
ICollection<PaymentAddress> CreatePaymentAddresses(int count, RootKey rootKey);
ICollection<PaymentAddress> CreatePaymentAddresses(int count, RootKey rootKey, NetworkType networkType);
}To use it with DI containers public void ConfigureServices(IServiceCollection services)
{
services.Configure<WalletConfig>(Configuration.GetSection("Wallet"));
services.AddSingleton<IAddressService, AddressService>();
}And provide the ability to configure services through {
"Wallet": {
"Network" : "Testnet",
"RootDirectory" : "/some/path/on/localhost",
"KeysDirectory" : "keys",
}
}public class AddressService : IAddressService
{
public AddressService(WalletConfig options)
{
NetworkType = options.Network;
//....
}
} |
Fair point. I don't know if I'm sold on a type so opinionated as My current thoughts around Types: public class PrivateKey
{
public byte[] Key { get; set; }
public byte[] ChainCode { get; set; }
//one option, could be redone with extensions
private PublicKey _publicKey { get; set; }
public PublicKey PublicKey {
get {
if(_publicKey == null) {
_publicKey = new PublicKey(Ed25519.GetPublicKey(Key), ChainCode);
}
return _publicKey;
}
}
}
public class PublicKey
{
public PublicKey(byte[] key, byte[] chaincode)
{
Key = key;
ChainCode = chaincode;
}
public byte[] Key { get; set; }
public byte[] ChainCode { get; set; }
}I could definitely see some very useful bit around having child types such as
Honestly I would just updated the "legacy support" to return the
These more opinionated methods are very "developer" friendly. This approach was more or less my thoughts for the empty
I like the idea of DI support but I think the @Daniel-SchaeferJ @safestak-keith If either of you two have an opinion on the matter, please chime in. |
...and, furthers the understanding for devs new to the matter.
Agreed,
I like this separation, makes total sense. And you are right, |
…lso updated the existing method to return Address model
yeah I think after the conversation I'm on board with this opinionated types.
100% agree. main reason why every service has an interface accompanying it. Alright so I'm down to start the type conversion process with One thing I'm thinking is just turning the |
I agree. |
* move encoding and decoding into partials to avoid large class
|
haha you beat me to the Bech32 static change. I was about to post a PR and wanted to reference the comment here, but say you already committed it |
| static byte[] ConvertBitsFast(ReadOnlySpan<byte> data, int fromBits, int toBits, bool pad = true) | ||
| { | ||
| // TODO: Optimize Looping | ||
| // We can use a method similar to BIP39 here to avoid the nested loop, usage of List, increase the speed, | ||
| // and shorten this function to 3 lines. | ||
| // Or convert to ulong[], loop through it (3 times) take 5 bits at a time or 8 bits at a time... | ||
| int acc = 0; | ||
| int bits = 0; | ||
| int maxv = (1 << toBits) - 1; | ||
| int maxacc = (1 << (fromBits + toBits - 1)) - 1; | ||
|
|
||
| List<byte> result = new List<byte>(); | ||
| foreach (var b in data) | ||
| { | ||
| // Speed doesn't matter for this class but we can skip this check for 8 to 5 conversion. | ||
| if ((b >> fromBits) > 0) | ||
| { | ||
| return null; | ||
| } | ||
| acc = ((acc << fromBits) | b) & maxacc; | ||
| bits += fromBits; | ||
| while (bits >= toBits) | ||
| { | ||
| bits -= toBits; | ||
| result.Add((byte)((acc >> bits) & maxv)); | ||
| } | ||
| } | ||
| if (pad) | ||
| { | ||
| if (bits > 0) | ||
| { | ||
| result.Add((byte)((acc << (toBits - bits)) & maxv)); | ||
| } | ||
| } | ||
| else if (bits >= fromBits || (byte)((acc << (toBits - bits)) & maxv) != 0) | ||
| { | ||
| return null; | ||
| } | ||
| return result.ToArray(); | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
was this copied from the Bech32 class? if so, should we just make it public so we dont duplicate
There was a problem hiding this comment.
yes i copied it.
I just put it there to play around with it (i.e. ReadOnlySpan<byte>) and saw the comment and added #15
but we can remove it.
There was a problem hiding this comment.
lets keep for now. after some experimenting/testing we can removed and update main method
|
Alright so unless you have any more ideas for this PR, I think we can look to merge it. I was speaking with @safestak-keith about the base types vs opinionated types, and I think we should move forward with base types then make a pass to add the opinionated types. you two made the point that it could help the developer understand the key/address derivations and keep them from implementing faulty processes. I agree with this but first want to establish the base types. |
Just one more idea that I want to raise. |
yeah that sounds good to me |
| if (data == null || data.Length == 0) | ||
| throw new ArgumentNullException(nameof(data), "Data can not be null or empty."); | ||
| if (!IsValidHrp(hrp)) | ||
| throw new FormatException("Invalid HRP."); |
There was a problem hiding this comment.
We might want to think about Custom Exceptions. There will be numerous time in our application that a witness, metadata, etc could be null so making some custom domain logic via Exceptions might work well for us
| if (b32Arr.Length < CheckSumSize) | ||
| { | ||
| throw new FormatException("Invalid data length."); | ||
| } | ||
| if (!VerifyChecksum(hrp, b32Arr)) | ||
| { | ||
| throw new FormatException("Invalid checksum."); | ||
| } |
There was a problem hiding this comment.
We should really make things like this into a Try/Parse pattern.
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance
I saw #5 and have a suggestions for Address Type.
I added some comments because I think it's important for new people to have some context.
I'd do more if this kind of contribution is welcome.