Authorized Client implementation #8
Conversation
| using OpenPayments.Sdk.HttpSignatureUtils; | ||
|
|
||
| /// <summary> | ||
| /// |
There was a problem hiding this comment.
Since this extension is public and not internal, lets define summary here
| if (keyBytes.Length != 32 && keyBytes.Length != 64) | ||
| if (keyBytes.Length != 32 && keyBytes.Length != 64 && keyBytes.Length != 48) | ||
| { | ||
| Console.WriteLine($"Key length was {keyBytes.Length} bytes, expected 32 or 64."); |
There was a problem hiding this comment.
Is is ok to use console.writeline in library?
There was a problem hiding this comment.
Forgot that while debugging, will use this opportunity to cleanup remaining temporary code.
golobitch
left a comment
golobitch
left a comment
There was a problem hiding this comment.
@cozminu without a doubt, this is an amazing job that you did! Thank you for this. I don't see any big problems with this implementation right now. Maybe I would have some nitpicks if I dive a little bit dipper but nothing to big.
I would however wait also on @koekiebox to take a look at this PR.
Thank you !
koekiebox
left a comment
koekiebox
left a comment
There was a problem hiding this comment.
Couple of comments, rest looks good @cozminu . 🚀 , some notes that may not be applicable:
- How much do we want to rely on comments? Or do we largely depend on openpayments.dev docs?
- Do we need to cover integration tests, and to which level?
There was a problem hiding this comment.
Can we add a .gitignore for the .idea folder and exclude? Specific to Jetbrains.
| if (keyBytes.Length != 32 && keyBytes.Length != 64) | ||
| if (keyBytes.Length != 32 && keyBytes.Length != 64 && keyBytes.Length != 48) | ||
| { | ||
| Console.WriteLine($"Key length was {keyBytes.Length} bytes, expected 32 or 64."); |
There was a problem hiding this comment.
The logic allows for 32, 64 and 48, but the console output only mentions 32 and 64.
|
|
||
| public AuthContractResolver() | ||
| { | ||
| // this.SetProp(typeof(Response), "Interact"); |
| MemberSerialization memberSerialization) | ||
| { | ||
| var prop = base.CreateProperty(member, memberSerialization); | ||
| prop.Required = Required.AllowNull; |
There was a problem hiding this comment.
I may have missed it, is there a way to control the JSON fields order?
| } | ||
| ); | ||
|
|
||
| // Console.WriteLine(JsonSerializer.Serialize(response, new JsonSerializerOptions { WriteIndented = true })); |
| { | ||
| Start = [Start.Redirect], | ||
| // Finish = new Finish() | ||
| // { |
| partial void PrepareRequest(HttpClient client, HttpRequestMessage request, | ||
| string url) | ||
| { | ||
| if (_privateKey == null || _keyId == null) return; |
There was a problem hiding this comment.
Do we want to log at least a warning here? Or is it handled elsewhere?
Addresses #2 , #3 , #4