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

85 Remove swap/buy for non-FLOW assets on Flow Wallet mobile #683

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

jeden
Copy link
Contributor

@jeden jeden commented Dec 18, 2024

Related Issue

Outblock/FRW#85

Summary of Changes

Need Regression Testing

  • Yes
  • No

Risk Assessment

  • Low
  • Medium
  • High

Additional Notes

N/A

Screenshots (if applicable)

N/A

@jeden jeden requested review from lmcmz and zhouxl December 18, 2024 21:44
@@ -43,7 +54,7 @@ extension WalletManager {

// MARK: - WalletManager

class WalletManager: ObservableObject {
class WalletManager: ObservableObject, TokenManager {
Copy link
Member

Choose a reason for hiding this comment

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

Let's create a new TokenManager class instead of mixing Token and Wallet up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Creating a separate class would require major refactoring, as its implementation references the two coin collections, which are defined and used in the wallet manager.
To be clear, this is feasible (and highly recommended), but not in the available time frame.

However, having a separate interface for token-related processing contributes to the separation of concerns, even if, under the hood, the implementation belongs to another manager. From a consumer point of view that is irrelevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GIven the changes described below, the TokenManager does not exist anymore

func isEvmToken(_ token: TokenModel) -> Bool {
guard self.isSelectedEVMAccount else { return false }
guard let evmSupportedCoins else { return false }
return evmSupportedCoins.contains(where: { token.contractId == $0.contractId })
Copy link
Member

Choose a reason for hiding this comment

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

Let's just check the token id only

Copy link
Member

Choose a reason for hiding this comment

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

The best way to check this, is add inga enum in TokenModel which have .evm and .cadence.
Cause those token fetch from different endpoint, we know if they are EVM or Cadence at the beginning.

struct TokenModel: Codable, Identifiable, Mockable {
let name: String
var address: FlowNetworkModel
let contractName: String
let storagePath: FlowTokenStoragePath
let decimal: Int
let icon: URL?
let symbol: String?
let website: URL?
let evmAddress: String?
var flowIdentifier: String?
var balance: BigUInt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could add a new field, but there are some issues:

  • Decoding of existing items (such as cached) will inevitably fail unless a default value is provided
  • To avoid the decoding failure, probably a manual implementation of encoding and decoding are required
  • There is at least one case where the type is unknown when using CustomToken, which is then converted to TokenModel.

I can't say if the type is relevant in the CustomToken case (if it is, then the new field should be optional), however I believe this is not a piece of information that belongs to the token itself, because it does not come from the source where the data is retrieved, but it is injected thereafter. A container should hold that information, which is actually what is currently done by using two separate collections.

However, If you believe this is needed, I can do it, but I need some clarifications to the three issues I mentioned above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok maybe I have found a way, I'll push a new commit shortly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lmcmz pls take a look at this commit and let me know if it makes sense. If not, I'll revert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants