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

feat_: share and parse transaction deep link #5959

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Cuteivist
Copy link
Contributor

@Cuteivist Cuteivist commented Oct 17, 2024

Task status-im/status-desktop#16412

User stories https://www.notion.so/Transaction-deep-link-10d8f96fb65c803c85fed5f4440ad439

Status-desktop change: status-im/status-desktop#16543

Changes:
Added generation of parsing of deep link for transactions
Updated unit tests

@status-im-auto
Copy link
Member

status-im-auto commented Oct 17, 2024

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 4c88939 #1 2024-10-17 11:02:21 ~5 min linux 📦zip
✔️ 4c88939 #1 2024-10-17 11:02:32 ~5 min ios 📦zip
✔️ 4c88939 #1 2024-10-17 11:02:51 ~5 min android 📦aar
✔️ 4c88939 #1 2024-10-17 11:03:04 ~5 min tests-rpc 📄log
✔️ 4c88939 #1 2024-10-17 11:30:35 ~33 min tests 📄log

Copy link
Collaborator

@igor-sirotin igor-sirotin left a comment

Choose a reason for hiding this comment

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

Is this going to be supported by status-web?

@@ -45,11 +45,21 @@ type ContactURLData struct {
PublicKey string `json:"publicKey"`
}

type TransactionURLData struct {
TxType int `json:"txType"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think tx is redundant here, it's all about the transaction?

Suggested change
TxType int `json:"txType"`
type int `json:"type"`

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 100% agree with you. The reason why I set it to txType to prevent any issues of type keyword that might happen on any layer.
While it might be redundant, I wanted to clearly point it is about transaction not url type or something like that.

@@ -302,6 +314,77 @@ func (m *Messenger) prepareEncodedCommunityChannelData(community *communities.Co
return encodedData, shortKey, nil
}

func (m *Messenger) ShareTransactionURL(request *requests.TransactionShareURL) (string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So to build the transaction URL, I need to know all details about it?
But most probably all of the details about the transaction are already somewhere in our database, can we just pass the transaction hash here and status-go will collect all required data?

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 feature creates link from the modal, not existing transaction. It is to request the payment by setting proper fields.

Please note that not all the fields are required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is to request the payment

wait, so there's no transaction yet? Should we call this transactionRequest then?
I can simply imagine a URL to the actual transaction 🤔

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 76.66667% with 14 lines in your changes missing coverage. Please review.

Project coverage is 47.45%. Comparing base (86cd41d) to head (4c88939).
Report is 3 commits behind head on develop.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
protocol/messenger_share_urls.go 77.77% 6 Missing and 6 partials ⚠️
services/ext/api.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #5959      +/-   ##
===========================================
+ Coverage    47.21%   47.45%   +0.23%     
===========================================
  Files          832      835       +3     
  Lines       137644   137789     +145     
===========================================
+ Hits         64985    65381     +396     
+ Misses       65175    64633     -542     
- Partials      7484     7775     +291     
Flag Coverage Δ
functional 10.43% <0.00%> (?)
unit 46.76% <76.66%> (-0.45%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
protocol/requests/transaction_share_url.go 100.00% <100.00%> (ø)
services/ext/api.go 6.26% <0.00%> (-0.02%) ⬇️
protocol/messenger_share_urls.go 67.91% <77.77%> (+1.43%) ⬆️

... and 131 files with indirect coverage changes

Copy link
Member

@jrainville jrainville left a comment

Choose a reason for hiding this comment

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

Nice work. Just one question

if r.TxType < 0 {
return ErrInvalidTransactionType
}

Copy link
Member

Choose a reason for hiding this comment

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

Should we also validate that the address is not empty for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. It might be changed by design team, but right now, deep link can be created without recipient. For example Swap doesn't have recipient.

I don't see the reason why I couldn't share tx setup for specific token, it is much easier than copy pasting name.

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.

4 participants