-
Notifications
You must be signed in to change notification settings - Fork 70
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
base: main
Are you sure you want to change the base?
Conversation
0f64ef1
to
5075bb6
Compare
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? |
Yeah, I don't think anyone should be actually implementing StellarAsset trait. |
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. |
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.