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

Include the token interface in the stellar asset interface #1427

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Jan 21, 2025

What

Include the token interface in the stellar asset interface, and therefore also in the client.

Why

The separation of the token functions from the stellar asset interface and client was originally done to ensure that developers didn't use the stellar asset client in situations where they really should be using the token client, to interface with tokens of any kind.

However, most of the time when people discover the stellar asset client is missing those functions, it creates confusion. It always requires an explanation. I think this is largely because in all other situations clients contain all the functions of a contract.

After discussing this with @tomerweller, who was the most recent person to run into this, I think while we had good intentions, it has landed with different impact than we thought it'd have.

How

The approach taken to the changes was to manually copy-paste the functions across the two interfaces. A macro could be used here but because any macro used here would need to interact well with the attribute macros in use on the traits, the complexity isn't worth it. It is unlikely the two will get out of sync because tests have been added to ensure the token spec is a subset of the stellar asset spec.

As part of the change I needed to fix a minor bug where the contractspec macro still wrote out a static variable even when exporting was disabled. Normally we don't allow more than one exported function with the same name per module. We have exporting turned off in this code, but the code was still generating the variable to export resulting in a collision since both interfaces contain functions with the same names.

@leighmcculloch leighmcculloch requested a review from dmkozh January 21, 2025 05:54
@leighmcculloch leighmcculloch marked this pull request as ready for review January 21, 2025 05:56
@leighmcculloch
Copy link
Member Author

There's a semver failure on the StellarAsset trait changing, but I'm unconvinced we should treat it like a breakage. The trait is very likely not implemented, unless someone is implementing it and the token trait to mirror the Stellar Asset Contract.

@dmkozh thoughts?

@dmkozh
Copy link
Contributor

dmkozh commented Jan 22, 2025

There's a semver failure on the StellarAsset trait changing, but I'm unconvinced we should treat it like a breakage. The trait is very likely not implemented, unless someone is implementing it and the token trait to mirror the Stellar Asset Contract.

Yeah, I don't think anyone should be actually implementing StellarAsset trait.

@leighmcculloch
Copy link
Member Author

I'm happy to hold this for protocol 23. This area of code is unlikely to get some churn. And it's the conservative approach given the semver. Also, the build won't let me merge while there's a semver breakage.

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.

2 participants