addition of c# wrapper with libs for windows and macos #66
Conversation
| agentIdentity.Did, agentIdentity.KeyName, Tools.EnsureHashPrefix(agentIdentity.Id), agentIdentity.Seed, | ||
| twinIdentity.Did, twinIdentity.KeyName, Tools.EnsureHashPrefix(twinIdentity.Id), twinIdentity.Seed, | ||
| Tools.EnsureHashPrefix(cDelegationName))); | ||
| if(result != null) { |
There was a problem hiding this comment.
- Shouldn't a level above be throwing exceptions and this class only is a pass-through wrapper?
Tools.Ensure*(I know I mentioned it elsewhere already, but) wouldn't it be better to just pass through and let the golang lib validate?
There was a problem hiding this comment.
I repeatedly endup writing the ensure in the application - i find it counter intuitive to have to prepend the string with a #
I am not strong about it but if you prefer not to do it in the Golang layer then we need to document it
There was a problem hiding this comment.
if you prefer not to do it in the Golang layer
Maybe I worded that badly - I am suggesting we do validate extra stuff in golang level (not even FFI) so that each of the FFI wrappers can be thinner and validation is consistent across languages.
| /// <returns>a new agent identity.</returns> | ||
| public AgentIdentity CreateAgentIdentity(string seed, string cKeyName, string cId) | ||
| { | ||
| string did = Tools.InvokeGoFunction(() => IdLib.CreateAgentIdentity(ResolverAddress, cKeyName, Tools.EnsureHashPrefix(cId), seed)); |
There was a problem hiding this comment.
The Tools.Ensure* usage: Surely the golang layer should be performing validation like this and if not we should add it and keep the wrappers as thin as possible. (The extra logic conditions would all need tests to verify)
There was a problem hiding this comment.
the golang validation kicks in if the # isn't present. I don't know why the library doesn't do it.
it's a bloody pain - so I decided to add it into the application layer.
but I choose not to touch the golang layer including ffi.
I am happy to remove it if it's a bother but then we need to document it somewhere for the users
There was a problem hiding this comment.
also - validation and input sanitisation should be done as early as possible
There was a problem hiding this comment.
validation and input sanitisation should be done as early as possible
Right, but C#/Java/FFI layers should be very thin so I'm not sure I agree that this means it should happen there. What's wrong with "throwing" things early on behind the actual golang API itself?
But I appreciate that's not done yet. This sounds like an improvement to make soon (but obv. not now).
| } | ||
|
|
||
| // Remove the trailing slash from the string | ||
| public static string RemoveTrailingSlash(string input) |
There was a problem hiding this comment.
Do we really need this? IIRC // or / shouldn't matter and the resolver should handle it fine anyway.
This again is extra logic in the wrapper which we shouldn't need (and if we do - why not make all of them behave the same by stuffing into the ffi or golang layer?
There was a problem hiding this comment.
I usually validate input as early as possible - I understand the concern and it's a good idea to validate in the ffi. but it's not bad to do it in the client either
There was a problem hiding this comment.
and it's a good idea to validate in the ffi.
I actually mean validate in golang, not FFI (keep ffi & users of FFI thin)
| return input.TrimEnd('/'); | ||
| } | ||
|
|
||
| public static string EnsureHashPrefix(string input) |
There was a problem hiding this comment.
This doesn't just ensure but also disallow a hash anywhere else. (As per other comments - why not remove this and let golang layer handle)
ffi/ffi_wrapper.go
Outdated
| @@ -328,7 +348,7 @@ func FreeUpCString(pointer *C.char) { | |||
| } | |||
|
|
|||
| func createIdentity( | |||
| isUser bool, // true for userId, false for agentId | |||
| idDoc idDocType, // true for userId, false for agentId | |||
There was a problem hiding this comment.
@StephanieBracaloni @vtermanis thank you for the review - I hope I have addressed all the points. Two important things
1 - java is only there because I use it in my code. I will create separately a branch and remove it since it's not for external parties (I am maintaining a branch in my personal github for expediency)
2 - this is the first release to a single non paying user of this library. I suggest we skip automation and proper release packaging until we have a second user, for expediency and time, also I am hoping we get some proper feedback on the API. If you feel strong about creating GH actions to build it, I'd be happy to contribute... I'll leave it to you and JJ to decide
3 - we still need to find a place where we can store the binary files. I originally asked @conbon to commit the binaries in the lib directory for simplicity - again if this is not desired we can do the GH actions.
@smartrics Thank you for addressing the review feedback. I completely understand the need to keep it simple and move fast but I don't 100% agree with the lack of testing because we only have a non-paying user because it will be a pain for a linux developer to fix something and be sure it still works, ... However this is just a bunch of recommendations since @JJCassidyIotics mentioned you will maintain the C# FFI wrapper and deal with the updates. |
|
I am happy to support this for the time being and understood that this requires testing and automation. As soon as we have a second user of this library, I'll hand it over to engineering for proper production ready release. |
Understood, this is a good compromise.
|
|
@smartrics - thank you for considering my feedback and it's great to have an initial C# option for users to trial. In summary (as you already know), my main two areas of concern are:
But I appreciate that for (1) we'd need to improve the Golang lib itself and (2) needs testing etc. I hope those we can consider in the near future. |
There was a problem hiding this comment.
✔️
- Please squash the commit history before merging (and ideally merge using rebase, not merge commits)
- See also addition of c# wrapper with libs for windows and macos #66 (comment)
No description provided.