-
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
Pool units in account portfolio #658
Pool units in account portfolio #658
Conversation
Add real image for the resource
Align the borders around tokens and nfts
Sources/Clients/AccountPortfoliosClient/AccountPortfoliosClient+Live.swift
Outdated
Show resolved
Hide resolved
associatedtype CodingKeyType: CodingKey = Self.CodingKeys | ||
} | ||
|
||
extension KeyedDecodingContainer { |
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 looks like it's doing something with decoding nested optionals, but how does it differ from the default behaviour?
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 is used to handle decoding of empty objects, for some reason the GW instead of sending back null, sends back an empty object. Which probably should be fixed on the backend side
extract( | ||
key: .validator, | ||
keyPath: \.asGlobalAddress, | ||
transform: ValidatorAddress.init(validatingAddress:) | ||
) | ||
} |
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 is very elegant, but if we want to use this pattern in even more places we could consider:
A. Turning extract into a property wrapper or macro
B. Defining a protocol to make the transform redundant, something like InitializableFromAddress
public typealias GetEntityDetails = @Sendable (_ addresses: [String]) async throws -> GatewayAPI.StateEntityDetailsResponse | ||
public typealias GetEntityMetdata = @Sendable (_ address: String) async throws -> GatewayAPI.EntityMetadataCollection | ||
public typealias GetEntityDetails = @Sendable (_ addresses: [String], _ explicitMetadata: [EntityMetadataKey], _ ledgerState: GatewayAPI.LedgerState?) async throws -> GatewayAPI.StateEntityDetailsResponse | ||
public typealias GetEntityMetdata = @Sendable (_ address: String, _ explicitMetadata: [EntityMetadataKey]) async throws -> GatewayAPI.EntityMetadataCollection |
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.
Elegant solution, even without the OptionSet...
Sources/Clients/GatewayAPI/GatewayAPIClient/GatewayAPIClient+Interface.swift
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.
Looks great. Maybe you should even spend the 5 minutes and turn the keys into an OptionalSet manually, it makes both logical and UX sense.
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.
Nice one! I requested a bunch of changes here and there, but some of them you can treat more as suggestions. It'd be also nice to retain the working condition of the AssetsFeaturePreview
🙏
Sources/Clients/AccountPortfoliosClient/AccountPortfoliosClient+Live.swift
Outdated
Show resolved
Hide resolved
Sources/Clients/AccountPortfoliosClient/AccountPortfoliosClient+Live.swift
Outdated
Show resolved
Hide resolved
Sources/Clients/AccountPortfoliosClient/AccountPortfoliosClient+Live.swift
Outdated
Show resolved
Hide resolved
Sources/Clients/AccountPortfoliosClient/AccountPortfoliosClient+Live.swift
Outdated
Show resolved
Hide resolved
Sources/Clients/AccountPortfoliosClient/AccountPortfoliosClient+Live.swift
Show resolved
Hide resolved
...eatures/AssetsFeature/Components/PoolUnitsList/Components/LSUResource/LSUResource+View.swift
Show resolved
Hide resolved
Sources/Features/AssetsFeature/Components/PoolUnitsList/Components/PoolUnit/PoolUnit+View.swift
Outdated
Show resolved
Hide resolved
Sources/Features/AssetsFeature/Components/PoolUnitsList/Components/PoolUnit/PoolUnit+View.swift
Outdated
Show resolved
Hide resolved
Sources/Features/AssetsFeature/Components/PoolUnitsList/Components/PoolUnit/PoolUnit+View.swift
Show resolved
Hide resolved
Sources/Features/AssetsFeature/Components/PoolUnitsList/PoolUnitsList.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.
Thanks for changes and comments 🙌
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.
One sec, the preview still doesn't work... It doesn't crash but displays no data. Looking 👀 Nevermind, I'll adjust the preview locally.
Okay, fixing that preview is not as straightforward as I thought 😬 I think we may want to have a broader architectural discussion at some point. For now, I'm going to leave it as is |
Added Pool Units to account portfolio.
How to test: