search

stellar / rs-soroban-sdk

Rust SDK for Soroban contracts.
Apache License 2.0
127 stars 67 forks source link

Remove spendable_balance from standard token interface #1080

Closed leighmcculloch closed 1 year ago

leighmcculloch commented 1 year ago

I also think spendable_balance should be in the StellarAssetInterface since it's only relevant to classic assets. What do you think?

Originally posted by @sisuresh in https://github.com/stellar/rs-soroban-sdk/pull/1079#pullrequestreview-1607318884

leighmcculloch commented 1 year ago

I also think spendable_balance should be in the StellarAssetInterface since it's only relevant to classic assets. What do you think?

I remember in the design discussion meetings somebody making this case, and somebody pushing back on it. If I recall correctly, I think because it would be more likely for contracts to be more interested in spendable balances than total balances, and so there's a benefit for all tokens supporting it. Otherwise, given the Stellar Asset being-first bias, it's possible contracts will depend on a Stellar Asset only function. i.e. Not including it in the token interface could harm interoperability.

I'll merge this as is, and open a separate issue about spendable balance.

Originally posted by @leighmcculloch in https://github.com/stellar/rs-soroban-sdk/issues/1079#issuecomment-1703087512

mootz12 commented 1 year ago

I would personally like to see the removal of spendable_balance from the token interface.

To me, balance represents the amount of tokens you can spend. It feels more appropriate to have the Stellar Asset Contract manage the compatibility concern by only returning the spendable_balance when balance is invoked.

A function like total_balance could be included in the StellarAssetInterface to return the balance + reserves/liabilities.

tomerweller commented 1 year ago

I think that both options can work and non is clearly better. Leaning towards the current implemented version because I perceive balance() as a function a wallet would invoke to display the total balance "owned" by the user, even if some is not usable due to liabilities/base reserve/etc. This is on par with what horizon calls a balance.

Regardless, this change has downstream effects and we're trying to avoid these in order to get a release out.