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

Automate import/export in the token contract #528

Closed
leighmcculloch opened this issue Oct 6, 2022 · 25 comments
Closed

Automate import/export in the token contract #528

leighmcculloch opened this issue Oct 6, 2022 · 25 comments

Comments

@leighmcculloch
Copy link
Member

leighmcculloch commented Oct 6, 2022

The manual import/export in the token contract encourages choice for the user about where they want to keep funds, which raises inconvenient decisions users need to make. For example, users can decide:

  • Where to keep their token values, in Stellar or in Soroban.
  • Where to send payments, to a users Stellar or Soroban balance.
  • How much to keep in Soroban to cover allowances.

The import/export that's required between Stellar and Soroban should for the most part be an implementation detail. It's the sort of thing that users shouldn't be exposed to and shouldn't care about. Wallets may be able to abstract this concept to some degree, but not broadly because wallets won't necessarily be able to make decisions about allowances, and they can't influence where others will send amounts.

We should consider automating import and export so that the token contract attempts to keep funds as much as possible in Stellar trust lines, and only backs out of doing so if doing so would exceed a trust line limit.

If we do that, it won't matter which way you send a payment, as the token contract will keep the balance in the same place. It won't matter where you keep a balance for allowances because an allowance could pull from the Stellar trust line balance. It won't matter if you use two wallets where one favors Soroban and the other Stellar.

As a concrete example what I'm proposing is:

  • If account holder A holds $100 in a Stellar trust line.
  • And if a transfer is to occur from A to B for $15 as part of a contract invocation that exercises an earlier allowance.
  • The token contract would import $15 from A's trust line, transfer it to B, and then export the amount back to B's trust line.

In the event a trust line limit would be exceeded the amount can be held on the Soroban side. It's an odd edge case, but it's below the surface and for the most part out of view of the user.

This proposal applies only to authenticated identifiers who are accounts (Stellar accounts) that exist at the time of the transaction. For accounts that don't exist, and for contracts and authenticated ed25519 keys, the balance would continue to be stored only in Soroban contract data ledger entries.

cc @tomerweller @nickgilbert @sisuresh @dmkozh @MonsieurNicolas @graydon

@leighmcculloch leighmcculloch changed the title Consider automating import/export in the token contract Automate import/export in the token contract Oct 6, 2022
@paulbellamy
Copy link
Contributor

Only having one balance seems like it would simplify a lot of client (wallet, dapp, etc) development.

I was just asking on discord, in fact, how a dev should tell if they need to (or are able to) import/export, and what the “rules” are for that.. Right now, afaict, the token contract doesn't expose any way to tell if a token wraps an asset or not.

The tricky bit with auto-import/export is that it makes the whole thing look much more seamless, but there are some tokens which can be used both in classic and soroban, and some which can only be used in soroban, which is a bit of a footgun potentially.

And in fact, you need a classic lumen balance to pay fees to use soroban, which is 🤯

@leighmcculloch
Copy link
Member Author

The tricky bit with auto-import/export is that it makes the whole thing look much more seamless, but there are some tokens which can be used both in classic and soroban, and some which can only be used in soroban, which is a bit of a footgun potentially.

I don't think this will be that big of a problem. How it occurs for me is that there are two APIs to tokens/assets, one is Stellar and only gives you access to Stellar Assets, the other is Soroban and gives you access to both Stellar Assets and custom tokens. Both are just APIs. Wallets can choose to use both APIs or one.

I think the footguns arise when an asset exists independently in both places by having balances in two places. If we omit that issue, then we reduce these two things to just two APIs for accessing the same thing.

@dmkozh
Copy link
Contributor

dmkozh commented Oct 6, 2022

I really like this idea from the usability standpoint.

There is a small performance/cost concern, as now every asset token operation would need to involve loading trustline entries, but likely it shouldn't be significant enough to matter.

@hanseartic
Copy link

hanseartic commented Oct 7, 2022

In the event a trust line limit would be exceeded the amount can be held on the Soroban side. It's an odd edge case, but it's below the surface and for the most part out of view of the user.

So that means a classic trustline's limit could be overruled by just sending through soroban? There may be cases where a limit is set on purpose. How are those addressed?

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Oct 7, 2022

So that means a classic trustline's limit could be overruled by just sending through soroban? There may be cases where a limit is set on purpose. How are those addressed?

I don't think that issue is addressed yet, however that issue is present with Soroban today regardless of this proposal. It might be worth opening a separate issue for that specific problem.

@mootz12
Copy link
Contributor

mootz12 commented Oct 7, 2022

Like the direction of this a lot!

Can you expand on how implementing something like this would effect, say, contracts/EOA's holding Stellar classic tokens in Soroban? Would account and trustline objects have to be created on the Stellar Classic side if one of these entities got tokens through Soroban? Or, would these balances be tracked ERC-20 style and avoid requiring classic intervention?

@leighmcculloch
Copy link
Member Author

That's a great question. This proposal applies only to authenticated identifiers who are Accounts (Stellar accounts) that exist at the time of the transaction. For accounts that don't exist, contracts, and for authenticated ed25519 keys the balance would be stored only in Soroban contract data ledger entries.

@dmkozh
Copy link
Contributor

dmkozh commented Oct 7, 2022

So that means a classic trustline's limit could be overruled by just sending through soroban? There may be cases where a limit is set on purpose. How are those addressed?

Just wanted to expand on this a bit more: currently the trustline limit can already be worked around with claimable balances. Basically from the classic trustline standpoint the invariant won't be broken. Creating 'above-limit' token balance in Soroban is pretty much equivalent to creating a claimable balance with the said amount (with the difference of it being automatically deposited to your trustline when it's below the limit again). I'm not sure if that can become a problem, but I'm not really sure what all the trustline limit use-cases are.

Also every classic trustline naturally has a limit of 2^63-1 (int64 max); Soroban balances are big ints, so they will be able to work around that limit too.

@mootz12
Copy link
Contributor

mootz12 commented Oct 12, 2022

@leighmcculloch how would a proposal like this effect both the parallel execution pipelines for Soroban <-> Classic and the parallel execution of contracts within Soroban?

It seems like the classic trustline resource could be under contention in both situations?

@dmkozh
Copy link
Contributor

dmkozh commented Oct 12, 2022

how would a proposal like this effect both the parallel execution pipelines for Soroban <-> Classic and the parallel execution of contracts within Soroban?

While parallel execution is still work in progress, I'm pretty sure that classic and Soroban transaction sets will be executed sequentially (e.g. classic txs sequentially first, then Soroban txs in parallel).

It seems like the classic trustline resource could be under contention in both situations?

Since token<->trustline mapping is 1:1, there is no additional contention for the trustlines in Soroban; if there is contention for the token for a given account, there just will also be contention for the respective trustline. So the degree of parallelism would stay the same, the only visible effect for the user would be potentially slightly higher gas fee as one more ledger entry is accessed (hopefully the difference will be marginal enough to not worry about it).

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Oct 12, 2022

It's also worth noting that in the worst case any contention already exists today with the existing design, because export and import still exists, it's just 100% in the hand of the account holder and the wallet to coordinate, where-as this proposal aims to have the platform coordinate it in the majority of cases. The mechanisms are the same, we're just changing whose responsibility it is to do them, with the goal of better interoperability up the stack by reduced need for off-chain coordination.

@dmkozh
Copy link
Contributor

dmkozh commented Oct 13, 2022

I've given this a bit more thought and unfortunately I think there are issues that are hard to work around without introducing inconsistency between classic asset tokens and regular smart tokens:

  • balance getter: if we just return a sum of smart + trustline balance, then due to existence of base reserve/liabilities for trustlines the users won't be able to perform operations that are close to the full balance amount (that would be inconsistent with pure smart tokens). If we return 'spendable' balance, then it becomes pretty confusing to use outside of contract calls (e.g. just to display the balance in the wallet), as it's no longer the total amount of token being held.
  • burn operation: classic trustlines may not even have clawback enabled and currently the ability to burn token can't be disabled (we probably should add that functionality btw). Even if we resolve the clawback issue, it would be really bad for the user if token admin could burn some amount from the classic trustline and it would be bad for the token admins to use burn or contract using burn as it no longer would behave consistently.

@leighmcculloch
Copy link
Member Author

Both of those problems exist conceptually from the users perspective today with the design we already have. The proposal only moves where that problem arises, so I don't think the existence of the problem should deter us from automating the actions.

For example, today balance returns only the token balance and not the asset balance. This means contracts will error if users tries to transfer or use an amount of their balance not spendable by the token. This is similar to contracts will error if user tries to transfer an amount that is not spendable for another reason. This is also not too dissimilar to user tries to import N while some amount of N is not spendable. Adding a spendable_balance function may be useful. Smart tokens can always return 0. Smart tokens may also develop their own logic for what is spendable or not, so they might return a value.

For example, the burn operation will not be able to burn classic assets held in a trust line today and will error if they attempt to burn more than is on the smart side. This is similar to a burn operation failing if the classic side has clawback disabled. On the classic clawback can already inconsistently for different of the same asset, because clawback can be enabled for some accounts and not others. So these types of possible failure exist for operators already. Also a user with clawback disabled on the classic side is going to be motived to always export anyway, so functionally I doubt there would be a significant difference.

@dmkozh
Copy link
Contributor

dmkozh commented Oct 13, 2022

For example, today balance returns only the token balance and not the asset balance.

But this behavior is consistent for any token; this is just the balance that token holds and anyone can expect it to behave in certain way (i.e. one could transfer all the balance etc.). Trustline currently just serves as an additional source/sink for the token balance and it doesn't modify the token behavior in any fashion. Arguably, they may not even need to be a part of the token interface as they are now, though changing that would probably be tricky/infeasible.

This is also not too dissimilar to user tries to import N while some amount of N is not spendable

That's true, however the important difference is user expectations. import`export` are functions that specifically deal with classic trustlines and they have a specific range of classic-related errors. All the other operations currently have exactly the same behavior and semantics for any sorts of tokens. If we start using classic balances everywhere, we leak the whole range of errors and classic intricacies into the token interface, which I'm not sure is a good idea.

For example, the burn operation will not be able to burn classic assets held in a trust line today

Which I think is the correct/expected behavior. Since classic asset and token may end up having different admins, having it behave differently looks like a security risk (and it also doesn't look right just from the UX standpoint).

This is similar to a burn operation failing if the classic side has clawback disabled.

This again would break expectations. E.g. imagine some scenario where I mint and then burn X amount of token. For regular tokens that's guaranteed to succeed (as long as there are no balance changes), but for asset tokens it won't. Also if part of balance happens to live on the smart side, then should it be burn-able? I agree these are edge cases and some of them could be irrelevant due to incentives, but I feel like this makes the whole user story of built-in token too complicated.

Again, the idea seems attractable on the surface, but I'm not sure if benefits outweigh potential confusion.

@kalepail
Copy link

I'm going to be honest I don't entirely follow the flow of this conversation but my only request is to ensure the import and export methods exist separate and invokable on the smart contract side in order to ensure support for importing a wrapped asset, doing some smart things, then exporting it again back to classic all in the same function call. I'm assuming that's possible under the current design and I would want that pattern to be maintained.

I understand the desire to obfuscate or join Stellar and Soroban balances into single balances or methods and some automation utilities around that are interesting but having these import and export functions be individual is an asset imo initially.

@leighmcculloch
Copy link
Member Author

ensure the import and export methods exist separate and invokable on the smart contract side in order to ensure support for importing a wrapped asset, doing some smart things, then exporting it again back to classic all in the same function call. I'm assuming that's possible under the current design and I would want that pattern to be maintained.

This is not a trivial execution to make because invoker auth doesn't flow down into sub-calls, so the only way to achieve this is with having the client do presigned auth for the account across the invocation, which is much more complex for the client at the moment.

This may be our first good argument for a use case for supporting multiple invocations in the Stellar transaction and treating them as atomic.

@dmkozh
Copy link
Contributor

dmkozh commented Oct 21, 2022

ensure the import and export methods exist separate and invokable on the smart contract side in order to ensure support for importing a wrapped asset, doing some smart things, then exporting it again back to classic all in the same function call.

FWIW the proposal would make this whole flow a no-op as the contract would just deal with a generic token interface. There are unfortunately the issues I've listed above that make implementing the proposal not as straightforward/feasible.

Also, classic wrapper tokens are just a single instance of a more general issue with sub-contract calls: e.g. any operation that needs to perform token transfer either needs to approve it (as a separate tx) and only then call into the contract. This is why I've been working on trying to simplify signature forwarding mechanism to be able to do authorized contract calls in a more general fashion.

This may be our first good argument for a use case for supporting multiple invocations in the Stellar transaction and treating them as atomic.

That's true, maybe we should try experimenting with doing multiple invocations within the same host instance. That shouldn't add complexity to metering/fees, as long as everything is shared.

@sisuresh
Copy link
Contributor

It sounds like we will either support multiple InvokeHostFunctionOp's in a single transaction, or provide some functionality in the host to do multiple invocations atomically, so we will have the ability to do import->action->export atomically (although I think the more common flow will be import->action or action->export).

I'm not really sure about automating import/export though. The protocol gets more complicated and the edge cases are weird. For example -

  • What do you do if contract A sends USDC to Stellar account B that does not have a frozen balance, but has a deauthorized trustline? Do you reject the transfer or keep the balance in B's Soroban balance?
    • I recognize that having trustline deauthorization and frozen balances be independent of each other is weird...
  • Another edge case that has already been mentioned in this thread is a trustline that is at it's limit.
    • This also needs to take liabilities into account, not just the trustline limit.

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Nov 14, 2022

What do you do if contract A sends USDC to Stellar account B that does not have a frozen balance, but has a deauthorized trustline? Do you reject the transfer or keep the balance in B's Soroban balance?

  • I recognize that having trustline deauthorization and frozen balances be independent of each other is weird...

I would expect it to behave the same as the built-in token already behaves with frozen balances: generate an error. Ref:

pub fn receive_balance(e: &Host, id: Identifier, amount: BigInt) -> Result<(), HostError> {
let balance = read_balance(e, id.clone())?;
let is_frozen = read_state(e, id.clone())?;
if is_frozen {
Err(e.err_status_msg(ContractError::BalanceFrozenError, "balance is frozen"))

Another edge case that has already been mentioned in this thread is a trustline that is at it's limit.

  • This also needs to take liabilities into account, not just the trustline limit.

This is not much different to hitting u128::MAX if we pick u128 as the amount type instead of BigInt. The moment we pick something like u64 or u128, we have an upper limit. So in this edge case we treat it like the limit has been reached.

@sisuresh
Copy link
Contributor

I would expect it to behave the same as the built-in token already behaves with frozen balances: generate an error.

This can be confusing for the user. They not only need to check if the frozen flag is set, but also check the trustline to see if it's deauthorized when trying to determine if a balance is frozen. The is_frozen method could also check the trustline flag, but this tying together of the token contract and trustlines increases the complexity of the token contract while also making it less transparent. Here are some of my concerns -

  • Changing the token contract admin does not stop the issuer from being able to "freeze" payments. A zero admin could mislead users into thinking that admin capabilities are disabled. These details may not be clear to users of the token.
  • If the Soroban user doesn't have a trustline on the classic side, then they no longer have to care about trustline flags. I wonder if this would incentivize users to transfer their balance away from Stellar accounts or delete their trustlines to avoid the de-authorization edge case.

Wrapped assets exist on other chains (like WETH). Do we know that the current UX around wrapping and unwrapping is a burden for users? I'm worried we're trying to solve something that isn't really a problem.

@MonsieurNicolas
Copy link
Contributor

+1 on maybe (re) stepping back: we have two balances not one for many reasons (one being the balance/number representations that are different between systems), and no matter what we do we'll always have edge cases where end users have to be aware of that (so it's not like we can have wallets that don't actually expose this information).

All those ideas that try to make it that the experience is like having one balance look to me like premature optimizations as they will have higher fee without actually understanding the various use cases, also note that for classic operations, there is really nothing we can do, so the solutions are not going to be symmetrical.

That said this problem is valuable to think about: if a wallet developer wanted to make the experience simpler, can they actually do it over time? I see this as a good "test case" for low level building blocks: like do we have a sane story for batching or for authorizing transfers?

@tomerweller
Copy link

tomerweller commented Nov 15, 2022

Wrapped assets exist on other chains (like WETH). Do we know that the current UX around wrapping and unwrapping is a burden for users? I'm worried we're trying to solve something that isn't really a problem.

I wouldn't say it's not a problem. Wrapping eth when going into DEFI is pretty idiosyncratic but dapps will usually guide you through the process. And then having both an ETH balance and a WETH balance is confusing to non defi natives (you might be able to mitigate with user experience. metamask definitely wasn't, the last time I checked)

I think that the problem is worse in Soroban/Stellar.

  1. All assets require wrapping/unwrapping. Not just the native currency.
  2. WETH was introduced fairly early in the ethereum ecosystem before major adoption. With Stellar There's an existing ecosystem of wallets and wallet-like services that support classic and, realistically, are not going to be quick to jump the soroban wagon. Our biggest win will be if these wallets could interoperate with soroban supported wallets seamlessly.

if a wallet developer wanted to make the experience simpler, can they actually do it over time?

Probably. And we need to try and outline some potential answers. But I'm concerned that for the sake of protocol simplicity we'll be dooming downstream systems to become very complex.

@JakeUrban
Copy link

disclaimer: I'm new here

What if Soroban accounts could only hold Soroban tokens, just like Stellar accounts can only hold Stellar assets, while Soroban contracts would still be able to hold balances of both wrapped Stellar assets and native Soroban tokens?

The primary (and significant) drawback here is that I wouldn't be able to send Stellar assets to someone who only has a Soroban account, so realistically this would mean all applications would need to support both Stellar and Soroban, instead of being a pure Soroban application.

But is that really a problem? It means that we'll almost certainly never see a massive ecosystem transition from Stellar to Soroban, but is that what we're trying to achieve? Or are we trying to give users of Stellar the ability to interact with smart contracts?

I'm not convinced this is the right approach, and it has significant implications for the future of both Stellar & Soroban, but I wanted to throw it out there.

@dmkozh
Copy link
Contributor

dmkozh commented Nov 15, 2022

What if Soroban accounts could only hold Soroban tokens,

There is no such thing as a 'Soroban account'. A balance on Soroban needs to be created by some contract, hence 'Soroban contracts would still be able to hold balances of both wrapped Stellar assets and native Soroban tokens' statement is equivalent to 'anyone can hold wrapped asset balances on Soroban'. What we could actually do is to limit the built-in token contract to only transfer wrapped assets between the classic accounts with the respective authorized trustline, but that greatly limits its usefulness and also doesn't really solve the usability issue that is being discussed here.

@sisuresh
Copy link
Contributor

Closing because we no longer have import/export functions. We updated the token contract to use a single balance for Stellar accounts - #615.

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

No branches or pull requests

10 participants