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

SEP-1: Allow externalization of currency metadata to other URLs. #1143

Closed
wants to merge 7 commits into from

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Mar 4, 2022

This is a follow-up to the discussion on the mailing list to extend SEP-1 by introducing a new top-level CURRENCY_TOMLS array in stellar.toml which must be a list of URLs pointing to [[CURRENCIES]] table entries.

@leighmcculloch
Copy link
Member

I think given this new proposal is substantially different to SEP-14, it should be a new SEP.

@leighmcculloch
Copy link
Member

leighmcculloch commented Mar 4, 2022

Actually, given the new proposal is a modification of SEP-1, we should minimally propose a change to SEP-1. We should keep it very minimal in this case, just adding the new field.

@Shaptic Shaptic dismissed a stale review via 870e54d March 7, 2022 19:19
@Shaptic Shaptic changed the title SEP-14 Refresh: Allow externalization of currency metadata to other URLs. SEP-1 Extension: Allow externalization of currency metadata to other URLs. Mar 7, 2022
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. I've made some suggestions inline.

@tomerweller Regarding #1143 (comment), do you have thoughts about how this change would be best introduced to the ecosystem? I think the change belongs to SEP-1 as it specifies the stellar.toml file. The SEP is an active SEP which means we can change it. The broad impact of this file is almost breaking in some ways, but technically it isn't. We will probably need a thoughtful rollout plan that is managed and tracked.

@Shaptic Could you update the full example at the end to demonstrate both ways to reference external files? Could you also update the version and the updated date in the pragma?

ecosystem/sep-0001.md Outdated Show resolved Hide resolved
ecosystem/sep-0001.md Outdated Show resolved Hide resolved
ecosystem/sep-0001.md Outdated Show resolved Hide resolved
ecosystem/sep-0001.md Outdated Show resolved Hide resolved
ecosystem/sep-0001.md Outdated Show resolved Hide resolved
ecosystem/sep-0001.md Outdated Show resolved Hide resolved
@leighmcculloch leighmcculloch changed the title SEP-1 Extension: Allow externalization of currency metadata to other URLs. SEP-1: Allow externalization of currency metadata to other URLs. Mar 7, 2022
Shaptic and others added 2 commits March 7, 2022 14:29
Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shaptic I left one suggestion, but regardless I think this is in a good place to start getting feedback from a wider audience.


Before this is merged, I think we need to identify a list of specific individuals who should be asked to review it.

@tomerweller @JakeUrban @marcelosalloum @lydiat Could you please review this change as a starting point?

If we have some general consensus then I think we should post it to the mailing list for feedback, and discuss if there is any other targeted feedback we should seek.

Anyone please post here if you think we need more or less process around this change.

ecosystem/sep-0001.md Outdated Show resolved Hide resolved
ecosystem/sep-0001.md Outdated Show resolved Hide resolved
Copy link
Contributor

@JakeUrban JakeUrban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the changes here allow for a single entity (platform / issuer / domain / etc) to define an infinite number of assets?

Maybe I'm misunderstanding the proposed solution, but if the root stellar.toml file has to list a URL for each page of currency entries, isn't there still a limitation based on the number of URLs you can fit into a single toml file?

I suggest we list a single CURRENCIES_LIST attribute on stellar.toml whose value is the URL for the first page of currencies. This page could then have a NEXT_PAGE pointing to the next page of currencies.

@Shaptic
Copy link
Contributor Author

Shaptic commented Mar 8, 2022

Do the changes here allow for a single entity (platform / issuer / domain / etc) to define an infinite number of assets?

Yes. You place them in however many TOML files you want as part of CURRENCIES_TOMLS.

If the root stellar.toml file has to list a URL for each page of currency entries, isn't there still a limitation based on the number of URLs you can fit into a single toml file?

You can put them all into one if you'd like; there are no size limitations on the external files. We wanted to avoid pagination as part of the spec to keep things simple: the list-based approach lets clients split their assets into however many files they want.

@leighmcculloch
Copy link
Member

leighmcculloch commented Mar 8, 2022

there are no size limitations on the external files

Hmm, there probably should be a size limit. Machines cannot load infinitely large files and depending on the TOML implementation may not be able to stream in and out contents to reduce memory usage. Some of these machines will be mobile devices.

Do the changes here allow for a single entity (platform / issuer / domain / etc) to define an infinite number of assets?

I think this highlights a lacking with the solution we have so far. You can define an infinitely large number of assets in the TOML files but the larger the set gets, the more inaccessible the set gets for a vast majority of clients, many of which are mobile devices with reduce memory and bandwidth capabilities.

That leads us to another problem. By introducing the pagination scheme only without any method to randomly access a single asset, there is no way to go to the CURRENCIES definition for a single asset, which is likely what most wallets do when looking up asset information for an asset held. Wallets can work around this today because an asset set is unlikely to be too large to enumerate. This proposal will make it possible for the asset set to be too large to enumerate.

@Shaptic
Copy link
Contributor Author

Shaptic commented Mar 8, 2022

I honestly don't see size/pagination as a problem for us to solve: if your deployed currencies file is 10MB, you're putting strain on your users and it's likely that your assets don't have as much reach, so it's in your best interest to break up your files in a way that makes your currencies accessible. Furthermore, you can already have ~14k assets in your base stellar.toml if you use the toml = "<url>" approach (napkin math), so I feel like we're generally trying to solve problems that don't exist yet.

If we do want to solve this in the SEP, there's a simple solution: require that each external TOML (a) follow stellar.toml size limitations of 100KB and (b) contain mutually exclusive entries. The latter is essentially pagination, and it leaves the implementation/splitting up to the author. But again, I don't think it's necessary unless we have demonstrable issues with this.

Edit: As for predictability / random access, we could require paginated currencies to be in sorted lexographically 🤷‍♂️

@JakeUrban
Copy link
Contributor

JakeUrban commented Mar 11, 2022

I don't have any objections to the changes made in the SEP, but I wonder whether or not the solution is sufficient for the applications that need this info. I think it is possible that we merge this only to eventually introduce another solution more akin to the original that @orbitlens designed.

The most glaring issue is the ability to quickly look up information on a specific asset. I'd imagine this kind of request is even more common that the request for all assets.

@orbitlens
Copy link
Contributor

orbitlens commented Mar 13, 2022

@leighmcculloch rightly pointed out that blowing up the size of stellar.toml leads to unpredictable resources usage on the client side. The 100KB limit is there for a reason.

if your deployed currencies file is 10MB, you're putting strain on your users and it's likely that your assets don't have as much reach, so it's in your best interest to break up your files in a way that makes your currencies accessible

This suggestion implies that all asset issuers are benevolent and intelligent. Which is not the case.

Using the proposed syntax, anybody will be able to execute a resource exhaustion attack that can affect any active user and almost any Stellar wallet. It is as simple as a) create a few thousands of garbage recurrent TOML files and b) send claimable balances to all Stellar accounts with recent on-chain activity. Once a user opens a wallet on a claimable balances page, the client will start downloading gigabytes of garbage.

And I'm not even talking here about various cases when asset developers may accidentally mess with TOML code generator leading to spontaneous problems for the clients. Please make no mistake, 100% of issuers with more than 100 assets will use automatic TOML generators because managing hundreds of assets manually is a completely stupid and prone to human errors task. I also totally agree with @JakeUrban regarding the paramount importance of the fast asset lookup ability, that has always been my primary argument.

All these considerations have been discussed in Slack and during protocol meetings more than three years ago, resulting in a SEP-14 proposal. This relatively simple proposal in a combination with a few tiny Core protocol modifications would have made Stellar an ideal platform for NFTs and on-chain stocks/commodities trading platforms back there, in 2019. And now we have lost all the potential of the leading NFT platform because we cannot even agree on a dynamic metadata SEP despite of 3 rounds of discussions that happened in 2018-2019 years.

SEP-1 by its nature provides a vast playground for various resource exhaustion and spoofing attacks because it doesn't lay restrictions on the external resources. For example, scam tokens often copy-paste asset logos and SEP-24 endpoint urls from trustworthy issuers, like UltraStellar or Circle to trick our asset ranking algorithm. Some asset issuers utilized huge images for asset logos, sometimes more than 10 MiB. Let's not aggravate the problem by providing even more opportunities.

@github-actions
Copy link

This pull request is stale because it has been open for 30 days with no activity. It will be closed in 30 days unless the stale label is removed.

@github-actions github-actions bot added the stale label Apr 12, 2022
@github-actions github-actions bot closed this May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants