feat: Transaction deep link sharing #16543
Conversation
Cuteivist
requested review from
micieslak,
a team,
caybro and
alexjba
as code owners
Cuteivist
requested review from
Khushboo-dev-cpp
and removed request for
a team
Jenkins BuildsClick to see older builds (14)
|
Cuteivist
force-pushed
the
feat/deep-link-16345
branch
from
6d2decd
to
aefe804
Compare
Cuteivist
force-pushed
the
feat/deep-link-16345
branch
from
aefe804
to
2000e11
Compare
@caybro Some SB were updated in https://github.com/status-im/status-desktop/pull/16555/files |
|
@Cuteivist please wait a bit with merging, review in progress. |
|
|
||
| states: State { | ||
| name: "success" | ||
| when: shareStateTimer.running |
There was a problem hiding this comment.
nice binding between state and timer :)
| onClicked: { | ||
| shareStateTimer.restart() | ||
| } |
There was a problem hiding this comment.
| onClicked: { | |
| shareStateTimer.restart() | |
| } | |
| onClicked: shareStateTimer.restart() |
Usually we keep single-liners without {}
| const url = root.swapAdaptor.swapStore.getShareTransactionUrl(Constants.SendType.Swap, | ||
| root.swapInputParamsForm.fromTokensKey, | ||
| root.swapInputParamsForm.fromTokenAmount, | ||
| root.swapInputParamsForm.selectedAccountAddress, | ||
| root.swapInputParamsForm.selectedNetworkChainId, | ||
| root.swapInputParamsForm.toTokenKey) |
There was a problem hiding this comment.
I know it's not matter of this PR, but please keep in mind to not put stores into adaptors.
| @@ -95,6 +96,7 @@ Item { | |||
| dappsEnabled: featureFlags ? featureFlags.dappsEnabled : false | |||
| swapEnabled: featureFlags ? featureFlags.swapEnabled : false | |||
| sendViaPersonalChatEnabled: featureFlags ? featureFlags.sendViaPersonalChatEnabled : false | |||
| transactionDeepLinkEnabled: featureFlags ? featureFlags.transactionDeepLinkEnabled : false | |||
There was a problem hiding this comment.
There is sth strange regarding that FeatureFlagsStore. It's object with empty, writable properties only.
It should be refactored to be as any other store. It means that access to the context property should be encapsulated in FeatureFlagsStore.qml and all properties marked as read only. Could you please do that small refactor there 🙏 ?
There was a problem hiding this comment.
It would be nice to add Storybook page for this new component. It's good to do it as a first step when creating new component bc it speed up development and further maintenance.
| return self.delegate.parseTransactionSharedUrl(url) |
There was a problem hiding this comment.
Is this method used somewhere?
Please keep standard empty line at the end.
| @@ -388,11 +390,47 @@ Item { | |||
| "" | |||
| ) | |||
| } | |||
|
|
|||
| function onShowTransactionModal(txType, asset, amount, address, chainId, toAsset) { | |||
There was a problem hiding this comment.
Requesting modals or other ui actions from the backend is an architectural problem. The problem begins in activateStatusDeepLink which parses the link and decides what kind of UI action should be done as a result.
The flow should be much simpler and separated here:
- backend exposes method to parse link returning json object (as a string bc of nim limitations)
- the method is wrapped in js function in a store. There the string response from original backend method is parsed to json and returns parsed form.
- UI calls this method exposed from store and decides on UI side what to do (calling modals, displaying details, etc)
I think we have three options here:
- refactoring existing
activateStatusDeepLinkaccording the the description above and adding the new functionality on top (biggest one) - exposing the method to parse the url and use it, ignoring the old flow, creating ticket to refactor it
- keeping it as it is but creating ticket to refactor it
Task #16412
User stories: https://www.notion.so/Transaction-deep-link-10d8f96fb65c803c85fed5f4440ad439
Status-go : status-im/status-go#5959
What does the PR do
NOTE that design is still in progress
Affected areas
Chat
Wallet
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality (including design for comparison)
tx_deep_link.mov
Impact on end user
What is the impact of these changes on the end user (before/after behaviour)
How to test
It would be best to follow testing through same code with included unfurling here #16541
Otherwise checkout to this branch status-im/status-go#5959 on status-go and set env variable
FLAG_TRANSACTION_DEEP_LINK_ENABLEDto "1"Send modal and swap modal generating the link.
Pasting the link into the chat.
Risk
Described potential risks and worst case scenarios.
Tick one: