Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

Refactor wrap/deploy of SAC #718

Merged
merged 13 commits into from
Feb 20, 2024
Merged

Conversation

tupui
Copy link
Contributor

@tupui tupui commented Feb 12, 2024

Closes #716

The "wrap" terminology has been changed to "deploy". This PR proposes a refactoring of the doc which correct the terminology. I also removed some duplication as to ease future maintenance. I also found it confusing to have multiple times the same info with slightly different explanation.

One thing I think should also be present is the clarification between token::Client and token::StellarAssetClient. But here I actually don't really get the difference 😅

In the doc example, there is this

// Create a client instance for the provided token identifier. If the
// `token_id` value corresponds to an SAC contract, then SAC implementation
// is used.

which to me means that I could just use token::Client. But if I do that, I can just burn but not mint. And with token::StellarAssetClient, I can mint but not burn.

Last thing, I am also not sure if the SAC guide should not be integrated into the SAC page itself.

Let me know how I should modify this. Please feel free to to anything with this PR or push directly on the branch 😃

cc @leighmcculloch

Copy link
Contributor

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Thanks for moving this guide forward. I left a couple comments.

docs/guides/cli/deploy-SAC.mdx Outdated Show resolved Hide resolved
docs/guides/cli/deploy-SAC.mdx Outdated Show resolved Hide resolved
docs/guides/cli/deploy-SAC.mdx Outdated Show resolved Hide resolved
docs/guides/cli/deploy-SAC.mdx Outdated Show resolved Hide resolved
docs/guides/cli/deploy-SAC.mdx Outdated Show resolved Hide resolved
docs/guides/cli/deploy-SAC.mdx Outdated Show resolved Hide resolved
docs/guides/cli/deploy-SAC.mdx Outdated Show resolved Hide resolved
docs/tutorials/tokens.mdx Outdated Show resolved Hide resolved
tupui and others added 2 commits February 13, 2024 17:42
Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
@tupui
Copy link
Contributor Author

tupui commented Feb 13, 2024

Thank you @leighmcculloch for the review. I addressed your suggestions 😃 Let me know if I can do more to help.

Copy link
Contributor

@ElliotFriend ElliotFriend left a comment

Choose a reason for hiding this comment

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

This is a really great PR! Thanks for the excellent additions!

docs/tokens/stellar-asset-contract.mdx Outdated Show resolved Hide resolved
docs/guides/cli/deploy-stellar-asset-contract.mdx Outdated Show resolved Hide resolved
@ElliotFriend
Copy link
Contributor

Hope you don't I pushed a couple commits to your branch:

  1. The build fails if there's any markdown syntax errors, so I linted those. (In this case, it was an extra space at the end of a line. Stupid nitpick, i know lol)
  2. I added an nginx redirect corresponding to the change in the filename

The PR looks good to me! If @leighmcculloch is happy with it, I'm happy with it!

Co-authored-by: Elliot Voris <elliot@voris.me>
@tupui
Copy link
Contributor Author

tupui commented Feb 14, 2024

Thank you @ElliotFriend for the review and the commits. I do prefer such active collaborations.

From my perspective I would still like to have a short description about

One thing I think should also be present is the clarification between token::Client and token::StellarAssetClient

But as I said above, I don't really understand it to explain it myself 😅

This could be kept for a follow up too of course.

Copy link
Contributor

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Couple small tweaks otherwise LGTM! Thank you!

docs/tutorials/tokens.mdx Outdated Show resolved Hide resolved
docs/tokens/stellar-asset-contract.mdx Outdated Show resolved Hide resolved
Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
@leighmcculloch
Copy link
Contributor

From my perspective I would still like to have a short description about

One thing I think should also be present is the clarification between token::Client and token::StellarAssetClient

We should say I think something along the lines that the token::Client is a client for accessing the functions exposed by any SEP-41 compatible token contract. The token::StellarAssetClient exposes the functions of the Stellar Asset Contract that are not part of SEP-41 and not expected or required to be found on compatible token contracts. So other contracts integrating with SEP-41 token contracts should ideally not dependent on the Stellar Asset Contract's additional functions, otherwise they won't be able to support custom tokens that implement SEP-41.

@tupui
Copy link
Contributor Author

tupui commented Feb 15, 2024

Oh and so this is why then burn and mint are not doubled in these 2 interfaces, they are exclusive. Got it, I will try to add that tomorrow (off for the day 😅, though feel free to not wait for me). But thanks, I thought I was doing something hacky by using two different clients! (From a pure DX perspective, that would be handy to have a client that would sort of merge both, if that makes sense.)

I think we need to add 2 paragraph: on the Token page discuss the token::Client and on the SAC page the token::StellarAssetInterface. Then on the SAC guide, we can discuss both clients with e.g. the mint/burn example.

@leighmcculloch
Copy link
Contributor

leighmcculloch commented Feb 15, 2024

From a pure DX perspective, that would be handy to have a client that would sort of merge both, if that makes sense.

I debated this, and I think it'd be convenient. My reason for not including it is I thought it would end up with folks writing code that uses the Stellar Asset Client everywhere, and never the token client, and so building integrations with tokens that are dependent on Stellar Assets by accident when they otherwise might work fine without the additional functions.

@tupui
Copy link
Contributor Author

tupui commented Feb 15, 2024

I did a suggestion for the clients. In the end, I only modified the SAC page, as I think it's fitting more. Otherwise the info would have been spread on 2 different pages, then it could be harder to connect the 2 things together.

building integrations with tokens that are dependent on Stellar Assets by accident when they otherwise might work fine without the additional functions

Is it because that could mean additional cost as the WASM would need to pack more code if only the SEP-41 part is needed?

@tupui
Copy link
Contributor Author

tupui commented Feb 19, 2024

@leighmcculloch @ElliotFriend is there anything else I can help with?

@leighmcculloch
Copy link
Contributor

I pushed a little tweak, will merge once verifying the preview deployment.

@leighmcculloch
Copy link
Contributor

cc @sisuresh

@leighmcculloch leighmcculloch merged commit e82542e into stellar-deprecated:main Feb 20, 2024
1 check passed
@leighmcculloch
Copy link
Contributor

@tupui Thanks for leading the charge on improving these docs!!!

@tupui tupui deleted the wrap_sac branch February 20, 2024 19:54
@tupui
Copy link
Contributor Author

tupui commented Feb 20, 2024

Thank you all for the help and collaborative work here, really enjoyed the process 😃 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove wrap token language
3 participants