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
Gh 716 push notifications #730
Conversation
Codecov Report
@@ Coverage Diff @@
## main #730 +/- ##
==========================================
+ Coverage 28.78% 29.85% +1.06%
==========================================
Files 324 328 +4
Lines 9262 9370 +108
==========================================
+ Hits 2666 2797 +131
+ Misses 6596 6573 -23
Continue to review full report at Codecov.
|
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.
I have several questions that need resolution
Multisig/Data/Services/Notification Service/RegisterNotificationTokenRequest.swift
Outdated
Show resolved
Hide resolved
Multisig/Data/Services/Notification Service/RegisterNotificationTokenRequest.swift
Outdated
Show resolved
Hide resolved
Multisig/Data/Services/Notification Service/RegisterNotificationTokenRequest.swift
Outdated
Show resolved
Hide resolved
Multisig/Data/Services/Notification Service/RegisterNotificationTokenRequest.swift
Show resolved
Hide resolved
Multisig/Data/Services/Notification Service/RegisterNotificationTokenRequest.swift
Outdated
Show resolved
Hide resolved
...isigTests/Data/Services/Safe Transaction Service/RegisterNotificationTokenRequestTests.swift
Show resolved
Hide resolved
] | ||
.joined() | ||
|
||
if let signature = try? Signer.sign(string).value { | ||
guard timestamp != nil else { | ||
throw "'timestamp' parameter is required if signing key exists" |
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.
maybe preconditionFailure()
is better because this would be a programmer's error
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.
ok
self.address = address.checksummed | ||
self.deviceID = deviceID.uuidString.lowercased() | ||
self.deviceID = deviceID |
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.
the lowercased()
got lost - either when this init is called or in here it should be present
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.
I checked - it worked fine. But I will make it lowercased()
@@ -17,6 +17,16 @@ struct UserDefault<T> { | |||
} | |||
} | |||
|
|||
@propertyWrapper | |||
struct UserDefaultWithDefault<T> { |
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.
not sure if we need this anymore
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.
👍
timestamp: String?) throws { | ||
|
||
guard UUID(uuidString: deviceID) != nil else { | ||
throw "'deviceID' should be UUID string" |
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.
I would make it preconditionFailure() or just precondition instead of guard because this is developer error
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.
ok
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.
thank you! 👏
No description provided.