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-0030 | deprecate signTx & add chain signing #206

Closed
wants to merge 2 commits into from

Conversation

SebastienGllmt
Copy link
Contributor

@SebastienGllmt SebastienGllmt commented Feb 1, 2022

The signTx endpoint for the dApp connector needs to change. The way we used to have a list of arguments with an optional argument makes any breaking change in the future extremely tedious. It's much better to just provide a JSON object so we can add new features to it

One of the reasons I want to do this is so that we can add the ability for dApps to request signing a chain of transactions. We can achieve this by having the dApp itself provide hints about the contents of the input for the next transaction even if they don't appear on-chain yet.

FAQ

Q: Can't wallets just keep track of pending txs so they know how to sign other transactions in the chain
A: This doesn't work in the generic sense. Here are some cases where this approach would fail:

  • Some of the inputs may not belong to the user (ex: the dApp may have its own transactions it needs to broadcast as part of its flow)
  • There is no guarantee the dApp submits transactions through the wallet's submit endpoint

@SebastienGllmt SebastienGllmt added the Update Adds content or significantly reworks an existing proposal label Feb 1, 2022
@SebastienGllmt SebastienGllmt self-assigned this Feb 1, 2022

- `partialSign`: if true, the wallet only tries to sign what it can. If false and the wallet could not sign the entire transaction, `TxSignError` shall be returned with the `ProofGeneration` code. Likewise if the user declined in either case it shall return the `UserDeclined` code. Only the portions of the witness set that were signed as a result of this call are returned to encourage dApps to verify the contents returned by this endpoint while building the final transaction.

- `inputHints`: to allow dApps to chain transactions together, they may need to provide to the wallet hints about some of the inputs of the transaction that are not on-chain yet. In order to do this in a trustless way, we have to provide the full transaction body (so that the wallet can hash the tx body and compare it to the hash in the inputs). Providing the full transaction instead of just the transaction body also allows access to any scripts or data in the witness that may be useful for the wallet. Note these hints could be use by the wallet to resolve regular transaction inputs, collateral or any other feature of Cardano in the future
Copy link
Contributor Author

@SebastienGllmt SebastienGllmt Feb 1, 2022

Choose a reason for hiding this comment

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

The main part I'm not entirely sure about is whether or not these input hints should be the full transaction or just the tx body. For chaining transactions, you just need the tx body. For things like data hints of script hints, we could make these separate fields instead of putting this all as part of the same field (esp. if we think providing hints for these things may require building mock transactions). Open to thoughts on this.

Copy link

Choose a reason for hiding this comment

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

I have a question, why can't we just sign continuing txs by calling signTx multiple times? Does the wallet implementation check for existing input and throw error if inputs don't exist whenever we call signTx? If this is the case, then isn't the solution is to change the wallet implementation to not check for existing inputs?
On a side note, to support chaining tx, I think we would also need support on the infrastructure side (i.e. Blockfrost) to submit multiple txs in a single API call to the same node

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@longngn if the wallet doesn't have access to the input, it can't know which input should be signed with which key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I asked around and it seems nobody needs the full tx so just the tx body should be enough for everyone

Choose a reason for hiding this comment

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

To present the user will all information about a transaction, including metadata, I would like to see transactions instead of transaction bodies as hints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MarcelKlammer do you have any use case in mind that would require seeing the metadata of the transaction that created the input that is being consumed in the tx?

Choose a reason for hiding this comment

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

Apart from Allowing to review all non on-chain transactions at once that let to this input, no.


- `partialSign`: if true, the wallet only tries to sign what it can. If false and the wallet could not sign the entire transaction, `TxSignError` shall be returned with the `ProofGeneration` code. Likewise if the user declined in either case it shall return the `UserDeclined` code. Only the portions of the witness set that were signed as a result of this call are returned to encourage dApps to verify the contents returned by this endpoint while building the final transaction.

- `inputHints`: to allow dApps to chain transactions together, they may need to provide to the wallet hints about some of the inputs of the transaction that are not on-chain yet. In order to do this in a trustless way, we have to provide the full transaction body (so that the wallet can hash the tx body and compare it to the hash in the inputs). Providing the full transaction instead of just the transaction body also allows access to any scripts or data in the witness that may be useful for the wallet. Note these hints could be use by the wallet to resolve regular transaction inputs, collateral or any other feature of Cardano in the future

Choose a reason for hiding this comment

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

Apart from Allowing to review all non on-chain transactions at once that let to this input, no.

@KtorZ KtorZ changed the title cip30: deprecate signTx & add chain signing CIP-0030 | deprecate signTx & add chain signing May 11, 2022
@mangelsjover
Copy link
Contributor

This is on hold until the governance path can be discussed on CIP 30.

@KtorZ
Copy link
Member

KtorZ commented Jan 17, 2023

@SebastienGllmt would you mind providing a quick update on that one? Is still relevant / has it been added to CIP-0030 implementations already?

@KtorZ KtorZ added the Category: Wallets Proposals belonging to the 'Wallets' category. label Mar 18, 2023
@SebastienGllmt
Copy link
Contributor Author

SebastienGllmt commented Mar 22, 2023

This is still an important CIP that should be implemented. It just got caught up in the CIP30 governance discussion and so it stalled waiting for a decision on that front despite the PR being approved by other wallet providers

@rphair
Copy link
Collaborator

rphair commented Mar 22, 2023

That's good to know @SebastienGllmt but then this needs a rebase on CIP-0030 in master particularly after the changes in #446. I tried to resolve the merge conflict but it would have involved taking material from both sides & so would have had to ask you to proofread the result anyway.

@Ryun1
Copy link
Collaborator

Ryun1 commented Jun 20, 2023

Hey @SebastienGllmt ,

This proposal was discussed today in CIP editors call 68. Are you still interested in pursing this amendment or perhaps there has been a work around discovered? because as it stands I do not think it is advisable to continue to alter CIP-30. Rather I would strongly recommend any more alterations to CIP-30 utilize the extensibility mechanism introduced in #446. This is to avoid the past issues of versioning of CIP-30 for both dApps and wallets, see #446 for discussions.

I would suggest packaging this in with other CIP-30 amendments (such as #443 and/or maybe #206) to create a multi-functional CIP-30 extension. I mentioned this idea in this issue comment. Would you be willing to pursue something like this?

@MartinSchere
Copy link

We would like to re-visit this PR. It's important now that more dApps are using transaction chaining.

@Unity-ADA
Copy link

We would like to re-visit this PR. It's important now that more dApps are using transaction chaining.

Agreed, this PR would benefit the whole of Cardano if it was merged

@Ryun1
Copy link
Collaborator

Ryun1 commented Mar 10, 2024

Although I agree that transaction chaining is a great feature to support, altering the CIP-30 base API would be a mistake, due to lack of versioning.
Instead, this would make a great CIP-30 extension, if someone wishes to take this forward.
Although, with the landscape of #587 this might not be needed.

Due to the age of this PR and it not fitting with the current landscape of CIP-30 extensions, I think we can move to close it cc @rphair @Crypto2099

@Ryun1 Ryun1 added the State: Waiting for Author Proposal showing lack of documented progress by authors. label Mar 10, 2024
@rphair
Copy link
Collaborator

rphair commented Mar 11, 2024

In sympathy with #209 (comment) and the last comments from @Ryun1 I'm closing this by quorum but please @Crypto2099, @SebastienGllmt please re-open (after a proper rebase) if there's a need & someone can carry this forward.

@rphair rphair closed this Mar 11, 2024
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. State: Waiting for Author Proposal showing lack of documented progress by authors. Update Adds content or significantly reworks an existing proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.