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

Show FT token standard attributes in post conditions when signing transaction #1700

Closed
markmhendrickson opened this issue Sep 10, 2021 · 40 comments
Assignees
Labels
area:post-conditions bug Functionality broken bug-p2 Critical functionality broken for few users, with no clear workarounds

Comments

@markmhendrickson
Copy link
Collaborator

As reported by @philipdesmedt, the post conditions UI doesn't show standard attributes for tokens – particularly decimal points, which leads the user to think they are setting a condition for value that's very different than reality, such as for WST here:

image

@philiphacks
Copy link

philiphacks commented Sep 13, 2021

Very odd behaviour.

This does not work (swap y for x)
image

This works (swap x for y, essentially the same code & front-end code as well)
image

I have to say the developer experience is really bad right now for post conditions. Hard to figure out what they should exactly look like and little documentation is provided (a lot of digging in the source code on github). Then you have these seemingly random bugs, combined with the removal of allow mode... I have to say these post conditions make me go wild, and not in the way I like it 😂

@markmhendrickson
Copy link
Collaborator Author

@philipdesmedt your latest screenshots here suggests we may have other work to do here than simply apply the FT standard to this view, since the values don't line up even when taking decimals into consideration (~478 DIKO instead of ~399 DIKO and ~30 USDA instead of ~25 USDA).

Are you seeing other discrepancies here that haven't caught my eye?

@markmhendrickson markmhendrickson added bug-p2 Critical functionality broken for few users, with no clear workarounds bug Functionality broken labels Sep 15, 2021
@philiphacks
Copy link

@markmhx the discrepancies are just because I added a multiplier and did "less or equal" in the post condition... the actual values are fine, the decimals are just not parsed AFAIK

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 22, 2021

@philipdesmedt I am trying to figure out how to reproduce this, can you confirm:

  1. What api server are you using? Is it up to date?
  2. What wallet version are you using?
  3. Can I get the contract id to check the token metadata endpoint?

Just confirming those things first, then is there somewhere I can test this currently?

@philiphacks
Copy link

  1. Hiro hosted & clarinet integrate (devnet) locally. Should all be up to date
  2. Latest .17 as of today on Brave
  3. Which contracts do you need?

@philiphacks
Copy link

You can test on testnet.arkadiko.finance

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 22, 2021

  1. Which contracts do you need?

Hoping for the contract id of the contract used in the images posted. Wanting to use here:
https://stacks-node-api.testnet.stacks.co/extended/v1/tokens/{contractId}/ft/metadata

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 23, 2021

@philiphacks
Copy link

@fbwoolf great!

Just adding again that the tokens are found correctly in one direction but not in the other (see #1700 (comment)). Hope this helps!

@philiphacks
Copy link

Another one that is not working.. I bid 31 USDA on the auction, not parsed in the post condition
image

I assume all these come from the same bug

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 23, 2021

@philipdesmedt can someone send DIKO and/or USDA to my testnet wallet?

@philiphacks
Copy link

@fbwoolf sure, can you drop your address here

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 23, 2021

@philipdesmedt thanks! ST12SYBBE6B5P3KQR49WH448HT80JVV645ZP6Y6WY

@philiphacks
Copy link

@fbwoolf sent you DIKO. Could not send USDA since it did not show up in the web wallet to send... another bug?

image

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 23, 2021

@fbwoolf sent you DIKO. Could not send USDA since it did not show up in the web wallet to send... another bug?

I believe we already have a fix in for this that just needs to be released. It is a caching issue (if same as reported from others).
#1708

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 23, 2021

If you sign out and then back in does it show up?

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 23, 2021

@philipdesmedt I can't seem to post liquidity in the app? What am I missing here?

Screen Shot 2021-09-23 at 1 09 35 PM

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 23, 2021

Also, can you get me the contract id? Need to verify the token metadata.

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 23, 2021

Also, can you get me the contract id? Need to verify the token metadata.

^ I can prob get that myself once I can get the app working, nvm.

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 23, 2021

I can recreate the issue now using USDA/DIKO add liquidity contract so all good.

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 23, 2021

@philipdesmedt finally have some answers for you ...did some digging. Using USDA/DIKO add liquidity contract found the following:

  • The post conditions we are getting are not accurate, see this info and will include screen shots of logs:
Post Condition:
assetName = "usda"
contractAddress = "ST26902A6NT1QSWQXXYM55EM579RY9CCPQDN02QW2"
contractName = ""arkadiko-token""

Post Condition:
assetName = "diko"
contractAddress = "ST26902A6NT1QSWQXXYM55EM579RY9CCPQDN02QW2"
contractName = "usda-token"

Post Condition:
assetName = "arkv1dikousda"
contractAddress = "ST26902A6NT1QSWQXXYM55EM579RY9CCPQDN02QW2"
contractName = "arkadiko-swap-token-diko-usda"

Asset Metadata:
name = "diko"
contractAddress = "ST26902A6NT1QSWQXXYM55EM579RY9CCPQDN02QW2"
contractName = "arkadiko-token"

In addition, our code will only match up assets the user has in their wallet. We don't request metadata for tokens they don't have (so it won't display accurately). This is an enhancement @aulneau has advocated for. I can create another issue for it, @markmhx? This is the code looking for the match (the asset is then undefined so we do not format it):

const asset = assets?.find(
    asset =>
      asset.contractAddress === contractAddress &&
      asset.contractName === contractName &&
      asset.name === assetName
  );

Screenshot of tx:
Screen Shot 2021-09-23 at 3 19 36 PM

Screenshots of the two post conditions for assets logged data:
Screen Shot 2021-09-23 at 3 04 57 PM

Screen Shot 2021-09-23 at 3 05 10 PM

Screenshot of my wallet DIKO token metadata:
Screen Shot 2021-09-23 at 3 05 23 PM

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 23, 2021

In summary, once the post conditions are correct on your end it should fix most of your issues. The only time you should still see the issue is if you don't have any of that particular token in your wallet.

@philiphacks
Copy link

@fbwoolf thanks!

could you point out exactly what is wrong (and how to fix it) though? The creation of post conditions is not very developer friendly atm... loads of trial and error, and if a little thing is wrong, it won't work apparently :)

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 23, 2021

@philipdesmedt I think in the particular example I sent just the assetName and contractName are getting mixed up on your end. Once that is fixed, it should be good I think. So, here in your code:

https://github.com/arkadiko-dao/arkadiko/blob/85415b765f5c3ca2fe4640a24cfd72781e433476/web/components/add-swap-liquidity.tsx#L199

These must be getting mixed up:

tokenXParam,
tokenX['name'].toLowerCase()

On our end we are receiving:

"arkadiko-token"
"usda"

And for Y:
https://github.com/arkadiko-dao/arkadiko/blob/85415b765f5c3ca2fe4640a24cfd72781e433476/web/components/add-swap-liquidity.tsx#L233

tokenYParam,
tokenY['name'].toLowerCase()

We are receiving:

"usda-token"
"diko"

I think once those are sent into createAssetInfo correctly, it will fix it. 🤞 Will need to check other places post conditions and used. It seems like just some of the X and Y traits are getting mixed up before they are used with createAssetInfo.

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 23, 2021

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 24, 2021

@aulneau also pointed out that the contractName is wrong in the logs I posted:

"arkadiko-swap-token-diko-usda"

This seems wrong here?
https://github.com/arkadiko-dao/arkadiko/blob/55d048043cda8a938e3319a17406b027c3e4d204/web/common/vault-utils.ts#L49

Used here:
https://github.com/arkadiko-dao/arkadiko/blob/85415b765f5c3ca2fe4640a24cfd72781e433476/web/components/add-swap-liquidity.tsx#L169

Both L45 and L49 have the same name and swap values.

@philiphacks
Copy link

@fbwoolf I tried your suggestions but I simply cannot figure things out, see screenshot

image

PR is here: arkadiko-dao/arkadiko#222

It is not recognising the tokens (i.e. not parsing the decimals). Any ideas?

@philiphacks
Copy link

@fbwoolf the transaction succeeds if I multiply the amounts by 1.2 (20% higher margin) and change to LessEqual btw.. but the numbers are still not parsed
image
image

So the TX is accepted which means the info passed to the PC is correct I assume? Seems like a bug somewhere still in the wallet parsing the decimals?

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 24, 2021

@philipdesmedt let me try your changes this morning. It seems like it should find those assets now in the conditional in our code. Let me look closer now that those changes have been made.

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 24, 2021

@philipdesmedt it doesn't look like what you posted has the correct contract address for the tokens (they are the same for both)? That needs to match with the metadata from the token:

ST26902A6NT1QSWQXXYM55EM579RY9CCPQDN02QW2
https://stacks-node-api.testnet.stacks.co/extended/v1/tokens/ft/metadata

We query this and match it to your post condition, then we use that info to display it in the UI. If they don't match, it returns undefined and the decimals won't get parsed correctly.

Where is the contract address coming from that you are using?

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 24, 2021

Looks like it is coming from an env variable?

@philiphacks
Copy link

philiphacks commented Sep 25, 2021

@fbwoolf the contract address is the standard deployer address that is specified locally in Devnet.toml... I am not using testnet, I am using a local environment (clarinet integrate) to debug this. The contract address is 100% sure correct.

@philiphacks
Copy link

@fbwoolf would it maybe be due to the fact that FT metadata processing is not enabled?

When going to http://localhost:3999/extended/v1/tokens/ft/metadata, I get

  "error": "FT metadata processing is not enabled on this server"
}```

@aulneau
Copy link
Contributor

aulneau commented Sep 25, 2021

@fbwoolf would it maybe be due to the fact that FT metadata processing is not enabled?

When going to http://localhost:3999/extended/v1/tokens/ft/metadata, I get

  "error": "FT metadata processing is not enabled on this server"
}```

This would 💯 cause it to fail in the ways you've described

@philiphacks
Copy link

@aulneau lol, damn :D sorry

can I enable it somewhere in settings?

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 25, 2021

Well, at least we are incrementally getting this figured out! Ha. I am not familiar with how to get this working correctly on devnet so I'll do some digging.

@philiphacks
Copy link

@fbwoolf that'd be awesome, thanks! I asked Ludo and he told me to use & specify a custom docker image in Devnet.toml but I don't really know where to start here lol

@philiphacks
Copy link

@fbwoolf @aulneau I merged my local branch and deployed to testnet, and can confirm that adding liquidity post conditions now parse as expected
image

Thanks! I think we'll have to figure out how to enable the FT metadata locally on devnet/mocknet, seems like something a lot of devs will run into as PCs need to be implemented more frequently.

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 25, 2021

@philipdesmedt this is fantastic! Happy this got resolved for you. I have been reading through the devnet docs this morning. I'll make sure to bring this up on Monday to see what changes we can make to help out other devs with this. 👍 @markmhx

@markmhendrickson
Copy link
Collaborator Author

@agraebe @pgray-hiro perhaps you'd like to open a separate related docs issue here for helping developers configure DevNet with FT metadata support for situations like this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:post-conditions bug Functionality broken bug-p2 Critical functionality broken for few users, with no clear workarounds
Projects
None yet
Development

No branches or pull requests

4 participants