search

anza-xyz / solana-pay

A new standard for decentralized payments.
https://solanapay.com
Apache License 2.0
1.29k stars 447 forks source link

[spec, possibly code] Rejecting excessive decimal places on `amount` #102

Open antonok-edm opened 2 years ago

antonok-edm commented 2 years ago

From the spec:

If the number of decimal places exceed what's supported for SOL (9) or the SPL token (mint specific), the wallet must reject the URL as malformed.

I noticed that the official @solana/pay JavaScript SDK will not reject a URL with more than the maximum decimal places in amount. It's not obvious from the spec, although it is totally understandable - it's impossible to implement this without on-chain data (at least in the SPL token case). Perhaps the spec should emphasize that this validation, both when generating and reading URLs, is the client's responsibility?

Still, regardless of the spec, there's no way for a downstream user of @solana/pay to detect the number of decimal places provided in the URL and thus reject any which are malformed. The amount field of ParsedURL is encoded as a BigNumber. That's an arbitrary-precision numerical representation, so it's possible to detect excessive nonzero digits. But as a parsing concern, these two URLs are indistinguishable:

solana:mvines9iiHiQTysrwkJjGf2gb9Ex9jXJX8ns3qwf2kN?amount=123.012 solana:mvines9iiHiQTysrwkJjGf2gb9Ex9jXJX8ns3qwf2kN?amount=123.01200000000000000000000000000000000000000

It is a reasonable interpretation of the current spec to only consider nonzero digits, but considering that clients are required to validate the number of decimal places anyways, my opinion is that the second case should be explicitly forbidden.

jordaaash commented 2 years ago

Hmm it's a valid question.

I guess it depends on whether we consider a trailing zero to be a decimal place? And it also depends on what you mean by the client here, but the validation is always the responsibility of the wallet in this case.

We do check for decimal places (probably after trailing zeros are omitted) in the reference implementation, e.g. https://github.com/solana-labs/solana-pay/blob/fb40bfc0a3a43824a4657b3d2a0ba67d3867c09f/core/src/createTransaction.ts#L96

But since this is up to wallets, I expect they will usually be looser than the spec. And it's still an open question of whether we need to explicitly disallow trailing zeros.

How these will be handled is probably specific to whatever language/numeric library the wallet is using, so I'm not sure it makes sense to be very strict?

jordaaash commented 2 years ago

Anyway, I'd be fine with saying

Trailing zero decimals must not be used.

or whatever you think makes sense. My guess is that most wallets won't check for this, but while we're creating reference implementations, it would be good to be consistent, and a stricter spec will help us do that.

antonok-edm commented 2 years ago

We do check for decimal places (probably after trailing zeros are omitted) in the reference implementation

Ah, I see now that it's in createTransaction - we've only been looking at parseURL so far, which still creates the parsed representation successfully. I think that all makes sense then.

Anyway, I'd be fine with saying

Trailing zero decimals must not be used.

or whatever you think makes sense. My guess is that most wallets won't check for this, but while we're creating reference implementations, it would be good to be consistent, and a stricter spec will help us do that.

Yes, I think that makes a lot of sense!