Stellar Asset Contract missing admin() and issuer() functions

[tamirms]

What problem does your feature solve?

There is no way to determine the admin of a stellar asset contract. Similarly, there is no way to determine the issuer of a stellar asset from except by calling the name() function and splitting on the ':.

What would you like to see?

Introduce an admin() function which returns the current admin of the Stellar Asset Contract.

Introduce an issuer() which returns the issuer of the stellar asset.

We should also consider renaming symbol() to code() so that it is consistent with the classic Stellar Asset semantics.

What alternatives are there?

As mentioned previously, the issuer can be determined by calling the name() function and splitting on the ':. I think it would be better to remove the name() function in favor of an issuer() function.

As for obtaining the admin, you can do it off chain by looking up the ledger entry containing the admin state. However, there isn't any way to recover this info from another soroban smart contract as far as I can tell.

The soroban implementation of starbridge is an example of a contract which would benefit by having an admin() or issuer() function

[dmkozh]

Could you please describe the use case in short? (the example PR is quite large and it's not immediately obvious where the admin would help).

The key distinction between name() and the potential admin() function is that the name() is immutable, while admin is mutable (and is not guaranteed to coincide with the issuer in the case of the classic assets). So I wonder if admin() is really ever useful. Also I'm not sure if we should add it to the generic token interface (what if the token has multiple admins?).

As for the issuer(), we could add it for the Stellar Asset Contract only, but it wouldn't be included into a general token interface, which would make it both harder to use the Stellar Asset Contract directly (as you need a separate spec) and to write interoperable contract (your contract will be locked into the classic assets). So this probably shouldn't happen, unless there is a really good reason for that.

[tamirms]

@dmkozh

Could you please describe the use case in short? (the example PR is quite large and it's not immediately obvious where the admin would help).

There is a soroban smart contract which represents a bridge between Stellar and Ethereum. A user can deposit assets on the bridge and then on ethereum there is a contract where the recipient of the bridge transfer can withdraw the asset which was deposited on the soroban smart contract. The same applies to the other direction (e.g. a user deposits on the ethereum bridge contract and withdraws the corresponding amount from the soroban contract).

Here are some example workflows:

1. Send XLM to Ethereum
   Alice deposits 10 xlm to the soroban smart contract.
   Bob (the recipient) shows this transaction to the bridge validator and receives signatures which can authorize a withdrawal.
   Bob calls the withdraw function on the ethereum smart contract and includes the validator signatures. The ethereum smart contract mints 10 wrapped xlm tokens to Bob's address.

2. Send ETH to Stellar
   Alice deposits 1 eth to the ethereum smart contract.
   Bob (the recipient) shows this transaction to the bridge validator and receives signatures which can authorize a withdrawal.
   Bob calls the withdraw function on the soroban smart contract and includes the validator signatures. The soroban smart contract mints 10 units of the wrapped ETH asset to Bob's address.

3. Send wrapped XLM from Ethereum to back Stellar
   Alice deposits 10 wrapped xlm tokens to the ethereum smart contract.
   Bob (the recipient) shows this transaction to the bridge validator and receives signatures which can authorize a withdrawal.
   Bob calls the withdraw function on the soroban smart contract and includes the validator signatures. The soroban smart contract transfers 10 xlm from its own balance to Bob's account.

Note how in (2) the soroban smart contract needs to mint the asset where as in (3) the smart contract does a normal asset transfer. An easy way to support both scenarios is if the soroban smart contract can call a function on the asset contract to determine if it is an admin. If the function returns true it will mint the asset otherwise it will call xfer().

Also I'm not sure if we should add it to the generic token interface (what if the token has multiple admins?).

Does it make sense for there to be a set_admin() function in the generic token interface without a corresponding is_admin() or get_admin() function which can be used to determine who is an admin?

[dmkozh]

A safer/more robust way to do either mint or xfer would be to call try_mint first and then call xfer if try_mint fails. This will account for other potential mint failure scenarios. But in your case you could also e.g. check for the contract's token balance and xfer if there is any (IIUC the token is either minted or stored, but never both).

Does it make sense for there to be a set_admin() function in the generic token interface without a corresponding is_admin() or get_admin() function which can be used to determine who is an admin?

That's a good question. I'm not sure if set_admin should be a part of the generic interface. I'm not sure what would be the reasons to implement custom tokens, but extension/modification of administrative functions seems like one of the directions. @sisuresh, do you have any thoughts on this?

[tamirms]

@dmkozh

A safer/more robust way to do either mint or xfer would be to call try_mint first and then call xfer if try_mint fails.

Can you explain how this is safer / more robust? I think calling try_mint could work but I don't see how it is safer or more robust. Keep in mind we only want to mint if and only if we are an admin of the asset contract.

But in your case you could also e.g. check for the contract's token balance and xfer if there is any (IIUC the token is either minted or stored, but never both).

No, there is no way to prevent someone from calling xfer() to transfer tokens to the admin of the token contract. If you xfer() to an issuer the tokens are burned (see https://github.com/stellar/rs-soroban-env/blob/main/soroban-env-host/src/native_contract/token/balance.rs#L241-L243 ) but that is not the case for a token admin.

[dmkozh]

Can you explain how this is safer / more robust?

This doesn't make any assumptions about the token administrative functions, implementation of mint etc. It's similar to any token operation: it's probably not a good idea to e.g. try checking the balances/allowances manually in order to check if certain transfer can clear. Just call it and check if it succeeds.

Keep in mind we only want to mint if and only if we are an admin of the asset contract.

That's one of the options. With auth next it's possible to e.g. have multiple bridge validators as token admins. It should be easy enough to pass the required signatures to mint on their behalf (so your contract doesn't necessarily have to be an admin). The approach of just calling what you need would work with this as well.

No, there is no way to prevent someone from calling xfer() to transfer tokens to the admin of the token contract.

I don't see how this is an issue, given that invariants won't be broken. But if you don't like it, the above approach still works.

[tamirms]

@dmkozh

This doesn't make any assumptions about the token administrative functions, implementation of mint etc.

Actually it does because you are assuming that mint will fail when called by a non-admin account. In my opinion, using an is_admin function is a more explicit check where as attempting to call mint is an indirect way of determining whether I am an admin.

That's one of the options. With auth next it's possible to e.g. have multiple bridge validators as token admins.

That is already the case. The bridge admin is a stellar account with multiple signers. Each signer of the account represents a different validator. The threshold on the account represents how many validators need to approve a withdrawal for it to succeed.

[tamirms]

I don't see how this is an issue, given that invariants won't be broken.

This invariant "you could also e.g. check for the contract's token balance and xfer if there is any (IIUC the token is either minted or stored, but never both)." is broken by the xfer() behavior that I described above

[dmkozh]

Actually it does because you are assuming that mint will fail when called by a non-admin account.

It doesn't make any assumptions like that. It doesn't really matter what is the auth on mint; the end result should be the same as long as the token follows the interface.

The bridge admin is a stellar account with multiple signers.

What I mean is that it's possible to forward the auth for the mint/xfer operation on behalf of arbitrary admin account and there is no need for the bridge contract to be the token admin. I don't know if that's necessarily a better option, just bringing it up.

This invariant "you could also e.g. check for the contract's token balance and xfer if there is any (IIUC the token is either minted or stored, but never both)." is broken by the xfer() behavior that I described above

I'm talking about your contract invariant, which IIUC is 'the user X has to receive a given amount of token Y'. It doesn't really matter if token Y has been minted or transferred from the contract's balance (the latter would be highly unlikely though). Again, that's just a possible option, but it doesn't seem like it breaks anything.

[tamirms]

the end result should be the same as long as the token follows the interface.

I think the issue here is that we have two different interpretations of the interface. Here is the token trait interface:

https://github.com/stellar/rs-soroban-env/blob/main/soroban-env-host/src/native_contract/token/contract.rs#L31-L131

Note how there exists a set_admin() function. According to my discussions with @sisuresh , set_admin() exists so that the "issuer" role of minting new tokens can be transferred to another account / smart contract.

It seems that you are suggesting set_admin() is a potentially undefined operation and I shouldn't assume that an admin role exists or that it is equivalent to having the ability to mint tokens.

I think it makes sense to include both set_admin() and is_admin() / get_admin() in the interface or none at all because having only set_admin() is unintuitive.

[dmkozh]

I think it makes sense to include both set_admin() and is_admin() / get_admin() in the interface or none at all because having only set_admin() is unintuitive.

I think we are on the same page here. Similar to initialization, I'm not sure if admin changes are abstract enough and are universally usable/useful. OTOH I also see the value of having both functions at the cost of restricting the implementers to a single admin. Maybe we should discuss this on Discord for the sake of getting more attention.

That said, I still think that in a lot of cases (including your particular case) there is nothing wrong about handling the contract call failures gracefully; that's the whole reason we have the try_ function variants in the client.

[tamirms]

OTOH I also see the value of having both functions at the cost of restricting the implementers to a single admin.

it is not necessary that we need to restrict implementers to a single admin. Consider the following two functions:

fn set_admin(
        e: &Host,
        admin: Signature,
        nonce: i128,
        new_admin: Identifier,
    ) -> Result<(), HostError>;

fn is_admin(
        e: &Host,
        id: Identifier,
    ) -> bool;

is_admin() is compatible with multiple admins (the function will return true for all admin accounts)

[dmkozh]

set_admin is the restrictive part here though.

[sisuresh]

So first, I should mention there's an issue open about adding a function to return the admin - #541.

I have been thinking about if there are any admin methods in the interface that should be pulled out because they're specific to Stellar assets and can be implemented as an extension to the interface (clawback() is a good example of this). I can see the argument for removing set_admin as well since administrative type methods are typically used as one-off calls (and are more likely to require custom logic/function signature). Custom tokens shouldn't have to implement methods that they don't need either. The EIP-20 interface is actually quite bare, which makes it easier to implement tokens.