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

Integrate a Stellar asset contracts #754

Merged
merged 12 commits into from
Jul 24, 2024

Conversation

tupui
Copy link
Contributor

@tupui tupui commented Jul 4, 2024

Closes #601
Closes #613

@tupui
Copy link
Contributor Author

tupui commented Jul 4, 2024

@briwylde08 I started the work.

I could not find any mention in the SDK of env.token.native_asset_contract. So I am not sure if that was maybe a remanent of something. Otherwise can you point me to the function in the SDK?

Otherwise let me know what you think 😃 I think I still want to show how to add in the test the check on the auth part.

@briwylde08
Copy link
Contributor

briwylde08 commented Jul 8, 2024

Hey @tupui! Can you comment on both of those issues so I can assign them to you? You're planning to do 603 and 601 in the same PR? Should we split them into separate PRs?

@tupui
Copy link
Contributor Author

tupui commented Jul 8, 2024

Sorry I made a typo (commented on #603 too) and this is not addressing #603 but #613.

For #601 I was saying in #613 that I think it sort of overlaps with #613. Also I cannot find the command to mint XLM in the SDK (as I said above), I just see the API to create generic clients which can represent any assets. Do you have some pointers here?

@briwylde08
Copy link
Contributor

@tupui, the issue numbers make more sense! @ElliotFriend any pointers here?

@ElliotFriend
Copy link
Contributor

@tupui Hi, there!

I think you're right in that there is some foreseeable overlap between #601 and #613. The discrepancy will only come in that env.register_stellar_asset_contract and similar functions are only available in tests.

As for the right command to use in the SDK, I believe I wrote env.token.native_asset_contract merely as a placeholder in the file a long time ago. I think it was as wrong then as it is now 🤣 .

I'm not entirely sure of the "right" way to use native XLM specifically in either scenario (testing or production). I know the native token contract uses the same Stellar Asset Contract as any other issued asset. So it might just be that an option (if it's not in fact the "right" way) would be to use the existing env.register_stellar_asset_contract function in tests and instantiate a client like let client = token::TokenClient::new(&env, &NATIVE_TOKEN_ADDRESS); in a production contract. A possibly outdated, but still probably pretty accurate, example is in Soroban Quest 6 where we create an "allowance" contract that is used to interact with the native XLM SAC.

That's probably the approach I'd take, but @leighmcculloch might have some more/better insight about whether or not there's a different way to interact with native XLM compared to other SACs.

@tupui
Copy link
Contributor Author

tupui commented Jul 10, 2024

Thank you for the inputs @ElliotFriend 🙇 I was frenetically going through the whole API doc trying to find something, glad I was not missing something in plain sight! 😅

I will have a look at this quest to improve what I have in this draft. Let me know otherwise if this has some big miss. It looks a bit slim, but with what I understand, I don't think there is much more to say (which would be pretty great as things don't have to be complicated!)

@leighmcculloch
Copy link
Member

The native token (XLM on pubnet) is just another stellar asset contract deployment, which is just an implementation of the SEP-41 token interface, so +1 you interact with the native token like any other token. One thing Soroban has attempted to address is any distinction between interacting with a native token vs an issued token vs a custom token, so there is very little special features surrounding the native token.

@tupui
Copy link
Contributor Author

tupui commented Jul 12, 2024

Marking this as ready for review. I am not sure much more is needed (I was thinking about auth, but then it seems out of place as it's not the focus and would be too distracting I think), so I would like to have your opinion here 😄

@tupui tupui marked this pull request as ready for review July 12, 2024 17:32
@briwylde08
Copy link
Contributor

Thanks @tupui! I'll get this assigned today.

@briwylde08 briwylde08 requested a review from ElliotFriend July 15, 2024 18:21
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.

Hi, @tupui!! Sorry for the delay in getting to this one!

It's looking really good! I've left a few suggestions, questions, thoughts, etc. throughout. Most of 'em are pretty small.

Overall, I really like the structure of what you've got here, and I think it'll work really well. Thanks for your awesome (as usual) work!!

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

As for the matter of auth, I think you're right about it not really fitting with the "main point" of what we're doing here.

@tupui
Copy link
Contributor Author

tupui commented Jul 22, 2024

Thank you @ElliotFriend for the review 😃 I think I addressed your suggestions. Let me know if you want me to adjust anything else.

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.

@tupui This is amazing!! Well done, my friend! I pushed a couple, very minor changes to the branch, and this is ready to go! Thanks so much!

@ElliotFriend ElliotFriend merged commit 7f5eb8f into stellar:main Jul 24, 2024
1 check passed
@briwylde08
Copy link
Contributor

Thank you @tupui! We'll take the award conversation back to the corresponding issue(s).

@tupui
Copy link
Contributor Author

tupui commented Jul 24, 2024

Thanks everyone! Awesome review and feedbacks as usual, it's very pleasant to work around here 😊

@tupui tupui deleted the 613_asset_to_contract branch July 24, 2024 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants