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

CIP-0111? | Web-Wallet Bridge - Wallet Transaction Caching #733

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

yHSJ
Copy link

@yHSJ yHSJ commented Jan 4, 2024

dApps on Cardano have been taking advantage of "transaction chaining," where a transaction spends outputs of a previous transaction that has not yet settled on chain. While this leads to a very smooth user experience, it also means users are getting used to signing transactions which have confusing or incorrect transaction summaries in the wallet UI.

It also allows scammers to take advantage of that confusing or incorrect transaction summary to build malicious chained transactions and trick users into signing transactions they did not want to approve.

This proposal introduces transaction caching into the wallet and requires that wallets display a clear warning when unable to provide a complete transaction summary.

Rendered

@Ryun1 Ryun1 changed the title Wallet Transaction Caching CIP-???? | Wallet Web-Bridge - Wallet Transaction Caching Jan 4, 2024
@Ryun1 Ryun1 changed the title CIP-???? | Wallet Web-Bridge - Wallet Transaction Caching CIP-???? | Web-Wallet Bridge - Wallet Transaction Caching Jan 4, 2024
@Ryun1 Ryun1 added the Category: Wallets Proposals belonging to the 'Wallets' category. label Jan 4, 2024
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

Nice addition to the Web-Wallet Bridge family. 👶

cip-smart-tx-signing/README.md Outdated Show resolved Hide resolved
cip-smart-tx-signing/README.md Outdated Show resolved Hide resolved
cip-smart-tx-signing/README.md Outdated Show resolved Hide resolved
cip-smart-tx-signing/README.md Outdated Show resolved Hide resolved
cip-smart-tx-signing/README.md Show resolved Hide resolved
cip-smart-tx-signing/README.md Outdated Show resolved Hide resolved
@klntsky
Copy link
Contributor

klntsky commented Jan 5, 2024

I propose a simpler solution: extend signTx with an optional argument of type [CborHex<Transaction>] which would include all the relevant not-yet-onchain context.

This approach would also cover

some transaction chaining done by dApps, where they reference UTxOs that were created in a transaction that didn't involve the user but hasn't settled yet

The wallets will always be able to reliably tell what is happening, including showing the entire chain (or, rather, DAG) of txs to the user.

@yHSJ
Copy link
Author

yHSJ commented Jan 5, 2024

I propose a simpler solution: extend signTx with an optional argument of type [CborHex<Transaction>] which would include all the relevant not-yet-onchain context.

Yes, this was another idea that I had that is a totally valid solution, and may be better. The key difference would be that it is then the responsibility of the dApp to cache transactions instead of the responsibility of the wallet. I felt that it made more sense to have transactions cached at the wallet level.

Consider the case where a user is using multiple dApps at one time. The caching of transactions at the wallet level also allows wallets to 1) remove UTxOs that are consumed in cached transactions but not yet on chain from the getUtxos response and 2) not let a user sign a transaction if a UTxO in a proposed transaction has been consumed in a cached transaction, preventing some of the "cardano-y" errors that users run into and become frustrated by.

I'm not sure that those are necessarily good ideas, hence why they weren't included in this CIP, but the caching of transactions at the wallet level has multiple benefits.

@NicholasMaselli
Copy link
Contributor

Adding comment of support because I saw this on twitter.

Yes we need some way for wallets to handle chained transactions. Now that there have been a few malicious wallet drainers in the ecosystem its very important for wallets to display exactly what is happening or put a warning (perhaps referring to chaining resources) if it cannot

@klntsky
Copy link
Contributor

klntsky commented Jan 9, 2024

@yHSJ

I felt that it made more sense to have transactions cached at the wallet level.

They will still be cached on the wallet side. dApps will be able to write to the cache by explicitly providing a set of utxos that should be considered spent after the user signs a given transaction. This is not, in fact, an approach different from caching, just a path to make caching work nicely with transaction chaining.

The caching of transactions at the wallet level also allows wallets to 1) remove UTxOs that are consumed in cached transactions but not yet on chain from the getUtxos response and 2) not let a user sign a transaction if a UTxO in a proposed transaction has been consumed in a cached transaction, preventing some of the "cardano-y" errors that users run into and become frustrated by.

Both cases could also be implemented with the extra argument approach.

1) only concerns wallet's address utxos, and the wallet can infer which of the utxos of the transactions provided via the extra argument are owned by it, and remove them from the set of available utxos


A wallet MUST maintain a cache of signed transactions that have not yet settled on chain. This cache MAY have a maximum size, and if the cache is full, the wallet MAY remove transactions from the cache, beginning with the oldest transaction to store a newer transaction. The wallet MAY remove cached transactions once they are seen on chain.

If a user signs a transaction through the [CIP-30 signTx](https://github.com/cardano-foundation/CIPs/blob/master/CIP-0030/README.md#apisigntxtx-cbortransaction-partialsign-bool--false-promisecbortransaction_witness_set) endpoint, the [CIP-103? signTxs](https://github.com/cardano-foundation/CIPs/blob/508ea0557bcd17d793da90312789165dcef8a4db/CIP-0103/README.md#apicip103signtxstxs-transactionsignaturerequest-promisecbortransaction_witness_set) endpoint, or any other CIP-30 extension that allows signing transactions, the wallet MUST cache the signed transaction with a TTL (time-to-live) greater than or equal to the [TTL of the transaction](https://github.com/IntersectMBO/cardano-ledger/blob/master/eras/babbage/impl/cddl-files/babbage.cddl#L57C8-L57C9). The wallet MUST NOT remove the transaction from the cache until the TTL has expired, the transaction has been seen on chain, or the cache is full and the wallet needs to remove transactions to make room for newer transactions.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think cache size is a concern here. The cache will only contain utxos owned by the wallet, so even if we consider ttl, it is safe to assume that the size will not grow beyond manageable values, as each utxo creation will incur costs. We already implicitly allow the wallet size to grow indefinitely (because the number of utxos in a multi-address wallet is unlimited).

If we allow valid implementations to drop cache data due to size limits, then a dApp would not be able to rely on presence of any data in the cache. And an implementation with SIZE=0 will be a valid one.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I missed this somehow. I think that it's really important to define this behavior in a standard. Lots of the current CIPs have undefined behaviors in edge cases and it can cause real issues across the ecosystem.

I want this standard to be comprehensive, regardless of device or wallet software. In the (rare, as you correctly point out) case that a cache has to be purged, it is important the wallet purges transactions in an appropriate manner (ascending chronologically).

It's an excellent point that a wallet could correctly implement this CIP with a cache size of zero bytes. I think it makes sense to add a "minimum cache size" that is n * maxTxSize. I would love to hear others' thoughts on what the value of n should be, since it will necessarily add (small) overhead to the wallet.

Copy link
Contributor

Choose a reason for hiding this comment

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

n * maxTxSize

Why do we need to cache whole transactions? I think it could work with caching just UTxOs, and only those that belong to one of the user's addresses.

Copy link
Author

@yHSJ yHSJ Jan 23, 2024

Choose a reason for hiding this comment

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

While the specific problem in the CIP is solved with just the UTxOs, I felt it may be useful to have the complete transaction. For example, caching the inputs as well would allow a wallet to refuse a signature on a transaction which consumes inputs which are already consumed in a cached transaction. Perhaps that is out of the scope of this CIP in particular though.

@yHSJ
Copy link
Author

yHSJ commented Jan 9, 2024

@yHSJ

I felt that it made more sense to have transactions cached at the wallet level.

They will still be cached on the wallet side. dApps will be able to write to the cache by explicitly providing a set of utxos that should be considered spent after the user signs a given transaction. This is not, in fact, an approach different from caching, just a path to make caching work nicely with transaction chaining.

The caching of transactions at the wallet level also allows wallets to 1) remove UTxOs that are consumed in cached transactions but not yet on chain from the getUtxos response and 2) not let a user sign a transaction if a UTxO in a proposed transaction has been consumed in a cached transaction, preventing some of the "cardano-y" errors that users run into and become frustrated by.

Both cases could also be implemented with the extra argument approach.

1) only concerns wallet's address utxos, and the wallet can infer which of the utxos of the transactions provided via the extra argument are owned by it, and remove them from the set of available utxos

I agree in general it makes sense to also support providing "context" at the sign endpoint level. I think the two ideas work well together, but it is important that a wallet does the caching as well. Otherwise, the attack vector would always exist (just don't pass in the optional additional transactions)

@Ryun1
Copy link
Collaborator

Ryun1 commented Jan 15, 2024

Should this be a CPS rather than a CIP?

@yHSJ
Copy link
Author

yHSJ commented Jan 15, 2024

Should this be a CPS rather than a CIP?

I think this particular proposal provides one solution to an (currently) unwritten CPS. I thought about writing a CPS first, which @Crypto2099 also suggested, but I felt that I'd rather not release the blueprint for a scammer without also providing a potential solution for wallets.

If a transaction contains an input that references an output that cannot be found, the wallet MUST display a warning to the user that the transaction includes unidentifiable inputs. The style and wording of this warning is left up to the wallet. The wallet MAY prevent the user from signing a transaction with unidentified inputs, but this may cause issues when interacting with some dApps.

## Rationale: how does this CIP achieve its goals?

Copy link
Collaborator

@Ryun1 Ryun1 Jan 23, 2024

Choose a reason for hiding this comment

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

I think it is worth discussing here the alternatives to this approach, such as stronger warnings within wallets OR not letting users sign unknown transactions.


## Abstract

This CIP extends [CIP-0030 Cardano dApp-Wallet Web Bridge](https://github.com/cardano-foundation/CIPs/tree/master/CIP-0030/) and [CIP-0103? | Web-Wallet Bridge - Bulk Transaction Signing](https://github.com/cardano-foundation/CIPs/pull/587) to allow wallets to provide a complete and accurate summary of transactions that spend outputs that don't exist on chain yet when possible, and clearly communicate to the user risks when they cannot.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would change this from being an explicit extension to these CIPs and rather be a best practice that applies to these other CIPs

Copy link
Author

Choose a reason for hiding this comment

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

I think I agree in general, I was just trying to follow the standards for "adding on" to the existing CIPs. That said, I think that it makes more sense to not explicitly extend any one CIP, especially as this CIP should be used along side ANY CIP that involves a wallet prompting users for a signature.

@rphair rphair changed the title CIP-???? | Web-Wallet Bridge - Wallet Transaction Caching CIP-0111? | Web-Wallet Bridge - Wallet Transaction Caching Jan 23, 2024
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

Agreed to promote at today's CIP meeting 🚀

cip-smart-tx-signing/README.md Outdated Show resolved Hide resolved
@NicholasMaselli
Copy link
Contributor

NicholasMaselli commented Jan 23, 2024

+1 from me. Caching is a good solution and this is definitely critical to Cardano UX.

Also it might be wise to add some method of adding transaction to the wallet cache. This can help sites that submit transactions through the their own backend

EDIT: This caches on sign transaction which is effectively what I wanted above

Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
Co-authored-by: Rhys Bartels-Waller <rhyslbw@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Wallets Proposals belonging to the 'Wallets' category.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants