rs-soroban-sdk icon indicating copy to clipboard operation
rs-soroban-sdk copied to clipboard

Include the token interface in the stellar asset interface

Open leighmcculloch opened this issue 1 year ago • 3 comments

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 avatar Jan 21 '25 05:01 leighmcculloch

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?

leighmcculloch avatar Jan 22 '25 07:01 leighmcculloch

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.

dmkozh avatar Jan 22 '25 17:01 dmkozh

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.

leighmcculloch avatar Feb 04 '25 05:02 leighmcculloch