Skip to content
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-1599] Change FactorSource into enum #534

Merged
merged 72 commits into from
Jun 2, 2023
Merged

Conversation

CyonAlexRDX
Copy link
Contributor

@CyonAlexRDX CyonAlexRDX commented May 30, 2023

ABW-1599

‼️See this comprehensive Confluence page with changes

  • Change FactorSourceID to contain one extra byte, which is an identifier of the FactorSourceKind
  • Change FactorSource to be an enum

Before this PR we had to load the whole factor source from factorSourcesClient in order to know the FactorSourceKind of a Factor Source for a certain FactorInstance, this PR makes it possible to know the FactorSourceKind directly from the FactorSourceID.

The primary concern I had earlier with using an enum for FactorSource was that each individual FactorSource type (struct ***FactorSource) would contain a lot of duplication of properties. This PR solves that by putting all the shared properties into common: Common, which seems like an OK trade off.

  • Import Olympia Software and Ledger
  • Add Another Ledger (not via Import Olympia)
  • Sign with Babylon software only (TX)
  • Sign with Olympia Software only (TX)
  • Sign with Olympia Ledger only (TX)
  • Sign with "Babylon Ledger" only (TX)
  • Sign with Babylon Software AND Olympia Software AND Olympia Ledger AND "Babylon Ledger" in same transaction, see TX, see Screen Recording (can skip to device signing, since Ledger takes long time...)
  • Add offDeviceMnemonic

@CyonAlexRDX CyonAlexRDX marked this pull request as ready for review May 31, 2023 10:17
@CyonAlexRDX CyonAlexRDX requested a review from kugel3 June 1, 2023 10:23
@CyonAlexRDX CyonAlexRDX removed the DO NOT MERGE Merging is blocked or prohibited due to missing specs or other higher priority PRs label Jun 2, 2023
Copy link
Contributor

@kugel3 kugel3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I don't have time to understand all the use sites, but it seems that it doesn't complicate things, and I trust you are using it correctly. The only thing is that would probably have a requirement func updateLastUsed(date: Date) so that var Common can be get only.

import Foundation

// MARK: - BaseFactorSourceProtocol
public protocol BaseFactorSourceProtocol {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this needs to include "Base"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a protocol BELOW FactorSourceProtocol which enum FactorSource conforms to. That enum cannot conform to FactorSourceProtocol since it cannot have static var kind: FactorSourceKind. Do you have a better name for it? :)