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] Handling FA2 and FA1.2 in history #104

Merged
merged 4 commits into from
Jul 4, 2023

Conversation

JulesGuesnon
Copy link
Collaborator

@JulesGuesnon JulesGuesnon commented Jun 19, 2023

Description

This PR detects when FA1.2 or FA2 tokens are sent to the wallet

Overview

image image

@JulesGuesnon JulesGuesnon self-assigned this Jun 20, 2023
@JulesGuesnon JulesGuesnon added the enhancement New feature or request label Jun 20, 2023
Copy link

@aguillon aguillon left a comment

Choose a reason for hiding this comment

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

I'm really not into React so I won't comment on your style or code correctness. I trust you about this!

I like that I can just transfer a token to a multisig from my wallet and that it appears in the history with the name I gave at the multisig creation and not my account address. However, in the future we're going to have people managing tokens with various external contracts (e.g., Youves vaults) and all they're going to read is KT1somethingsomething. We should start thinking about extending aliases now, for instance by using the same aliases as tzkt. This isn't urgent, but I'm curious about your take on this.

I don't think the way tokens are displayed during proposals or after executing them is good though:
proposal

2

I also think you want to show the token's icon somewhere in the proposal or history. I haven't tested with artsy NFTs, but I guess that it only gives you the token ID and contract address, and that won't cut it for most users.

Other bugs:

  • What's the "metadata" field for? Why is amount = 0? I'm testing here with a FA2 token. It seems to work better for FA1.2.
  • Let's assume I have 2 FA1.2 tokens. When submitting a proposal, if I try to transfer 1 to one address, and 2 to another address (which can't work because 1 + 2 = 3 > 2) then I can still submit the proposal, and, later, sign it. Obviously I then can't execute it correctly.
  • When making a proposal, if I add a "Transfer FA2" item and fill the "address" field first, and then select a token, I get an "Invalid address" message below the address. Removing one character and then putting it back fixes this, but it's still a minor UI bug.

I can open separate issues for these bugs if you prefer.

Comment on lines +140 to +144
x.token.standard === "fa2"
? TransferType.FA2
: x.token.standard === "fa1.2"
? TransferType.FA1_2
: TransferType.UNKNOWN,

Choose a reason for hiding this comment

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

I didn't read the rest of the code (TL;DR, sorry sorry) but how do you decide the standard of a token in TzSafe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's provided by tzkt api. which actually led to some weird bug because tzkt was identifying some contract being fa2 when it's wasn't compliant with the spec

components/HistoryToken.tsx Outdated Show resolved Hide resolved
@rueshyna
Copy link
Collaborator

@aguillon

for alias, I opened the issue: #106

@JulesGuesnon If the followings are easy to fix, please process. if not, I'm ok with creating an issue for tracking except the last one.

I also think you want to show the token's icon somewhere in the proposal or history. I haven't tested with artsy NFTs, but I guess that it only gives you the token ID and contract address, and that won't cut it for most users.

I agree that having icon would be nicer.

  • What's the "metadata" field for? Why is amount = 0? I'm testing here with a FA2 token. It seems to work better for FA1.2.

The metadata is for advance users who create their own customized lambda. If users create proposals by tzsafe-ui, metadata is always empty now. the amount of FA2 is showing in Params/Tokens. if users transfer multiple FA2 tokens, it's a bit hard to show in the current UI.
I do agree we should have nicer presentation for regular users. The current proposal presentation is still toward developers. I think this one is a bit out of scope of this PR.

  • Let's assume I have 2 FA1.2 tokens. When submitting a proposal, if I try to transfer 1 to one address, and 2 to another address (which can't work because 1 + 2 = 3 > 2) then I can still submit the proposal, and, later, sign it. Obviously I then can't execute it correctly.

When the time users create a proposal, it doesn't really execute. Therefore, users have time to prepare funding more FA1.2 token before resolving the proposals. I think showing warnings for users would be good, not restrict them to create these kinds of proposals. It's the same for XTZ transfer.

  • When making a proposal, if I add a "Transfer FA2" item and fill the "address" field first, and then select a token, I get an "Invalid address" message below the address. Removing one character and then putting it back fixes this, but it's still a minor UI bug.

It's a bug. should be fixed.

Copy link
Collaborator

@rueshyna rueshyna left a comment

Choose a reason for hiding this comment

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

In general, it looks good to me

pages/history.tsx Show resolved Hide resolved
components/HistoryToken.tsx Outdated Show resolved Hide resolved
components/HistoryToken.tsx Outdated Show resolved Hide resolved
components/HistoryToken.tsx Outdated Show resolved Hide resolved
pages/history.tsx Show resolved Hide resolved
@JulesGuesnon
Copy link
Collaborator Author

@rueshyna

I agree that having icon would be nicer.

This is easy to add because I do have the data to do it, but my main problem is basically where to add it. I quickly thought about it, and it doesn't really fit with current history design I think

It's a bug. should be fixed.

On it

For the rest I think it can be turned into issues

@rueshyna
Copy link
Collaborator

rueshyna commented Jul 3, 2023

This is easy to add because I do have the data to do it, but my main problem is basically where to add it. I quickly thought about it, and it doesn't really fit with current history design I think

That's what I was thinking of. issue: #108

Copy link
Collaborator

@rueshyna rueshyna left a comment

Choose a reason for hiding this comment

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

👍

@JulesGuesnon
Copy link
Collaborator Author

@aguillon

I don't think the way tokens are displayed during proposals or after executing them is good though:

I agree with this, there is probably a way more visual solution to come up with as we have icons for FA1.2 and FA2. Maybe rather than a big table that is definitely made for developers, we could show some small cards for each transaction. And each card could have an icon:

  • Token icon if there's some
  • Custom icon for Tzsafe related operations (adding user, deleting user, updating proposal duration, etc...)

the same aliases as tzkt.

I see the point, and I think it would be valuable to have that kind of feature, but I don't really see how we can use it with our current features. I mean, it can probably be used to have a feature of verified address, and provide more informations to the user, but as long as addresses in TzSafe are only strings that you fill into inputs it's quite complicated I think. The only way I can see an integration of this feature for now, is fetching an API to see if the address is verified on tzkt, and provide a link to the user if it exists so he can check by himself

@JulesGuesnon JulesGuesnon merged commit 49ad102 into main Jul 4, 2023
1 check passed
JulesGuesnon added a commit that referenced this pull request Jul 4, 2023
@rueshyna rueshyna deleted the jules/feature/received-fa2-fa1.2 branch July 4, 2023 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants