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.

Sign up for GitHub

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

Jump to bottom

Feature/ABW-1288 Actions for addresses #450

Merged
merged 1 commit into from
May 3, 2023
Merged

Feature/ABW-1288 Actions for addresses #450

merged 1 commit into from
May 3, 2023

Conversation

nikola-milicevic
Copy link
Contributor

@nikola-milicevic nikola-milicevic commented Apr 20, 2023 •
edited
Loading

Jira ticket: https://radixdlt.atlassian.net/browse/ABW-1288

Description

This PR includes:

  • refactored AddressView that is self contained and encapsulates copy address and view on dashboard functionalities; it can be used in any View or Feature, and it doesn't require any special integration
  • removed AccountLabel and AccountButton components
  • added SmallAddressView component, defined in the design guide
  • removed pasteboardClient from every feature, now it's only used in AddressView
  • cleanup and simplification, there's less code added than deleted

Notes

I started implementing this by creating a Feature, but it turned out that using such feature would add some amount of boilerplate code and unnecessary complexity. Also, integrating it into other subviews which aren't Features themselves would introduce unnecessary work and complexity without gain.

As previously agreed, this component is currently agnostic about the address input type, and contains functionalities similar to ones found here and here to perform internal logic. Let me know if you think this should change. Please keep in mind there should be a differentiation between account addresses, resource addresses, non-fungible global IDs, component addresses, and transaction IDs, with special case for viewing NFTs on dashboard (local vs global ID).

How to test

Copy address

  1. Tap on account address, or tap and hold to see the context menu and tap on Copy
  2. Paste the address anywhere outside the app
  3. Verify pasted address matches the copied address

View on Dashboard

  1. Tap and hold on address and tap on View on Dashboard
  2. Verify web browser opens correct page displaying correct info

Screenshots

Copy Address Copy NFT ID
Screen Shot 2023-05-01 at 13 38 53 Screen Shot 2023-05-01 at 13 40 55

Video

Simulator.Screen.Recording.-.iPhone.14.-.2023-04-20.at.13.49.01_480p.mov

PR submission checklist

  • I have tested account to account transfer flow and have confirmed that it works

@nikola-milicevic nikola-milicevic self-assigned this Apr 20, 2023
kugel3
kugel3 reviewed Apr 20, 2023
View reviewed changes
.../TransactionReviewFeature/TransactionReviewGuarantees/TransactionReviewGuarantees+View.swift
@@ -120,11 +124,9 @@ extension TransactionReviewGuarantee {
}

public var body: some SwiftUI.View {
WithViewStore(store, observe: \.viewState, send: { .view($0) }) { viewStore in
WithViewStore(store, observe: \.viewState) { viewStore in
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it's even actionless now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, you mean to add .actionless to the store? Is there some performance gain?

kugel3
kugel3 approved these changes Apr 20, 2023
View reviewed changes
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.

Great overall, lots of nice deletions. But I thought you said that it had to be a feature because in some cases the parent did need to know that you copied the address?

Also, since we will definitely want to show some kind of notification, do you have a plac for how to do that when it's just a standalone view? Using Preferences? Or some kind of in place notification?

GhenadieVP
GhenadieVP reviewed Apr 21, 2023
View reviewed changes
Sources/Core/FeaturePrelude/AddressView.swift Outdated Show resolved Hide resolved
Sources/Core/FeaturePrelude/AddressView.swift Outdated

private enum DashboardConstants {
static let scheme = "https"
static let host = "betanet-dashboard.radixdlt.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

probably rcnet, not?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have existing address types that we should probably use here, rather than strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rcnet was not working at the time, now it is. Also, since there was no existing type I've added Radix.Dashboard.

@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1288-actions-for-addresses branch 6 times, most recently from 5e12956 to 5d50ea4 Compare April 26, 2023 14:30
kugel3
kugel3 reviewed Apr 26, 2023
View reviewed changes
Sources/Core/FeaturePrelude/AddressView/AddressView.swift Outdated
}

private var addressView: some View {
Text((addressString ?? "").formatted(format))
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think that it should be addresses that we format, not strings

kugel3
kugel3 reviewed Apr 26, 2023
View reviewed changes
Sources/Core/FeaturePrelude/AddressView/AddressView.swift Outdated
Image(asset: action == .copy ? AssetResource.copy : AssetResource.iconLinkOut)
}

private var addressString: String? {
Copy link
Contributor

Choose a reason for hiding this comment

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

And it shouldn't be done in the View, it's a model thing, that is quite general

Copy link
Contributor

Choose a reason for hiding this comment

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

this also makes it clear it should not be a protocol, but an enum... if we need TXID to be case, then I suggest this:

this view changes name ot LedgerIdentifiableView and the enum will be LedgerIdentifiable:

/// A globally unique and stable identifier to some data on the immutable ledger shared across the Radix decentralised network.
enum LedgerIdentifiable {
    case transactionIdentifier(TXID)
    case address(Address)
}

where Address is an enum bridging all our addresses together

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suffixes like able are used for protocols, but we can try for this as well.

kugel3
kugel3 reviewed Apr 26, 2023
View reviewed changes
Sources/Core/FeaturePrelude/AddressView/AddressView.swift Outdated

// MARK: - AddressView
public struct AddressView: SwiftUI.View, Sendable {
let address: AddressType
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we want a new address protocol, but either way this should be any AddressType, right?

CyonAlexRDX
CyonAlexRDX requested changes Apr 27, 2023
View reviewed changes
Sources/EngineToolkit/EngineToolkitModels/Models/Transaction/Transaction/TXID.swift Outdated
@@ -7,3 +7,6 @@ extension TransactionIntent {
}

public typealias TXID = TransactionIntent.TXID

// MARK: AddressType
extension TXID: AddressType {}
Copy link
Contributor

Choose a reason for hiding this comment

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

A Transasction Identifier is not an Address, if TXID need conformance to some protocol, the name AddressType is the wrong name.

Sources/Core/FeaturePrelude/AddressView/AddressView.swift Outdated
Image(asset: action == .copy ? AssetResource.copy : AssetResource.iconLinkOut)
}

private var addressString: String? {
Copy link
Contributor

Choose a reason for hiding this comment

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

this also makes it clear it should not be a protocol, but an enum... if we need TXID to be case, then I suggest this:

this view changes name ot LedgerIdentifiableView and the enum will be LedgerIdentifiable:

/// A globally unique and stable identifier to some data on the immutable ledger shared across the Radix decentralised network.
enum LedgerIdentifiable {
    case transactionIdentifier(TXID)
    case address(Address)
}

where Address is an enum bridging all our addresses together

@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1288-actions-for-addresses branch 2 times, most recently from 55a0fea to 5fa8839 Compare May 1, 2023 10:35
@nikola-milicevic
Copy link
Contributor Author

I updated the code with the new UI for context menu and added screenshots.

nikola-milicevic
nikola-milicevic commented May 1, 2023
View reviewed changes
Sources/Core/SharedModels/LedgerIdentifiable.swift
Comment on lines +39 to +47
public var addressPrefix: String {
switch self {
case .transaction:
return "transaction"
case .nonFungibleGlobalID:
return "nft"
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed for constructing the correct URL.

Sources/Core/SharedModels/LedgerIdentifiable.swift
Comment on lines +68 to +79
public var addressPrefix: String {
switch self {
case .account:
return "account"
case .package:
return "package"
case .resource:
return "resource"
case .component:
return "component"
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed for constructing the correct URL.

@nikola-milicevic nikola-milicevic added the DO NOT MERGE Merging is blocked or prohibited due to missing specs or other higher priority PRs label May 1, 2023
@nikola-milicevic
Copy link
Contributor Author

Blocked by #459

@nikola-milicevic nikola-milicevic mentioned this pull request May 1, 2023
Merged
@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1288-actions-for-addresses branch 6 times, most recently from a780d91 to 3ca7321 Compare May 3, 2023 11:42
@nikola-milicevic
Add AddressComponent
d3f77dc 
Use address component in home screen

Use address component in new entity

Use address component in fungible token details

Use address component in nft details

wip

wip

Replace AddressComponent with AddressView

Fix tests

Refactor

Minor updates

wip

wip

Move to enum

Clean up

Update context menu

Use existing init
@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1288-actions-for-addresses branch from 3ca7321 to d3f77dc Compare May 3, 2023 12:19
@nikola-milicevic nikola-milicevic removed the DO NOT MERGE Merging is blocked or prohibited due to missing specs or other higher priority PRs label May 3, 2023
@nikola-milicevic
Copy link
Contributor Author

Not blocked, ready for merging. After #459 is merged, it should be refactored so we keep the current functionality, but remove or simplify LedgerIdentifiable so it contains new address types.

CyonAlexRDX
CyonAlexRDX approved these changes May 3, 2023
View reviewed changes
Copy link
Contributor

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

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

Approved given we have a Jira ticket and plan for updating the code once RET/address improvments have been done

@nikola-milicevic
Copy link
Contributor Author

Jira ticket for the upcoming task: https://radixdlt.atlassian.net/browse/ABW-1458

@nikola-milicevic nikola-milicevic merged commit affbb14 into main May 3, 2023
@nikola-milicevic nikola-milicevic deleted the feature/ABW-1288-actions-for-addresses branch May 3, 2023 14:38
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyonAlexRDX CyonAlexRDX CyonAlexRDX approved these changes

@GhenadieVP GhenadieVP Awaiting requested review from GhenadieVP

@kugel3 kugel3 Awaiting requested review from kugel3

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nikola-milicevic @CyonAlexRDX @GhenadieVP @kugel3