-
Notifications
You must be signed in to change notification settings - Fork 9
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
[ABW-1268] CAP33 parse imported payloads #438
Conversation
Sources/Clients/ImportLegacyWalletClient/ImportLegacyWalletClient+Interface.swift
Show resolved
Hide resolved
Sources/Clients/ImportLegacyWalletClient/ImportLegacyWalletClient+Interface.swift
Show resolved
Hide resolved
Sources/Clients/ImportLegacyWalletClient/ParseOlympiaPayloads.swift
Outdated
Show resolved
Hide resolved
Sources/Clients/ImportLegacyWalletClient/ParseOlympiaPayloads.swift
Outdated
Show resolved
Hide resolved
Sources/Clients/ImportLegacyWalletClient/ParseOlympiaPayloads.swift
Outdated
Show resolved
Hide resolved
Sources/Clients/ImportLegacyWalletClient/ParseOlympiaPayloads.swift
Outdated
Show resolved
Hide resolved
Sources/Clients/ImportLegacyWalletClient/ParseOlympiaPayloads.swift
Outdated
Show resolved
Hide resolved
Sources/Clients/ImportLegacyWalletClient/ParseOlympiaPayloads.swift
Outdated
Show resolved
Hide resolved
Sources/Clients/ImportLegacyWalletClient/ParseOlympiaPayloads.swift
Outdated
Show resolved
Hide resolved
} | ||
|
||
// MARK: Parse Account | ||
private static func _deserializeAccount( |
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.
This function feels like it should almost be using a RegexBuilder.
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 did start impl using PF SwiftParsing, but they have rewritten it for Swift 5.8 and not worth having to change the impl once we are in Swift 5.8....
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.
Ah, but RegexBuilders are forever. And very easy to use.
var truncated = String(name.rawValue.prefix(30)) | ||
let forbiddenCharacters: [String] = Olympia.Export.Separator.allCases | ||
for forbiddenChar in forbiddenCharacters { | ||
truncated = truncated.replacingOccurrences(of: forbiddenChar, with: Olympia.Export.accountNameForbiddenCharReplacement) | ||
} | ||
return truncated |
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.
Since we're replacing all forbidden characters with the same thing, I think it is clearer to do it in one go, something like
public static func _sanitize(name: NonEmptyString?) -> String {
guard let name else { return "" }
let result = name.rawValue.prefix(30).replacing(
Olympia.Export.Separator.regex,
with: Olympia.Export.accountNameForbiddenCharReplacement
)
return String(result)
}
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.
where
static public var regex: some RegexComponent {
ChoiceOf {
inter
intra
headerEnd
accountNameEnd
}
}
Or some more compact version
Sources/Clients/ImportLegacyWalletClient/ParseOlympiaPayloads.swift
Outdated
Show resolved
Hide resolved
Sources/Clients/ImportLegacyWalletClient/ParseOlympiaPayloads.swift
Outdated
Show resolved
Hide resolved
try Self.deserialize( | ||
payloads: .init(rawValue: OrderedSet( | ||
uncheckedUniqueElements: payloadsStrings.compactMap { NonEmptyString(rawValue: $0) } | ||
))! |
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.
This scares me, could we replace it with a guard-throw, since the function is throwing?
Sources/Clients/ImportLegacyWalletClient/ParseOlympiaPayloads.swift
Outdated
Show resolved
Hide resolved
Sources/Clients/ImportLegacyWalletClient/ParseOlympiaPayloads.swift
Outdated
Show resolved
Hide resolved
Sources/Clients/ImportLegacyWalletClient/ParseOlympiaPayloads.swift
Outdated
Show resolved
Hide resolved
Sources/Clients/ImportLegacyWalletClient/ParseOlympiaPayloads.swift
Outdated
Show resolved
Hide resolved
...rtFromOlympiaLegacyWallet/Children/ScanMultipleQRCodes/ScanMultipleOlympiaQRCodes+View.swift
Outdated
Show resolved
Hide resolved
.../ImportFromOlympiaLegacyWallet/Children/ScanMultipleQRCodes/ScanMultipleOlympiaQRCodes.swift
Outdated
Show resolved
Hide resolved
.../ImportFromOlympiaLegacyWallet/Children/ScanMultipleQRCodes/ScanMultipleOlympiaQRCodes.swift
Show resolved
Hide resolved
Tests/Clients/ImportLegacyWalletClientTests/ImportLegacyWalletClientTests.swift
Outdated
Show resolved
Hide resolved
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.
Very cool. I haven't tested it, I suppose I can't without an olympia account, or can I make one now? Just some small stylistic comments.
Jira ticket: ABW-1286
Description
This is an implementation of serialization and deserialization of CAP-33 the import string format between Olympia and Babylon wallets.
N.B. that Babylon clients only need to deserialize (parse), but I've implemented serialize so that I could generate test vectors for our consultants who maintain the Olympia wallet can use to verify their serialize implementation, as well as our Android client can use to verify its implementation of deserialize.