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

Sign transaction updates #1097

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

Sign transaction updates #1097

wants to merge 5 commits into from

Conversation

quietbits
Copy link
Contributor

@quietbits quietbits commented Oct 10, 2024

Updated sign transaction logic:

  • Have arrays of decorated signatures for every signature type (secret key, hardware wallet, extension wallet).
    • Secret key and hardware wallet already provide signatures.
    • Extension wallet provides signed transaction XDR. We use it to extract the last signature. If, in the future, extension wallets can return signatures, it will be an easy update.
  • Every signature type signs the imported transaction (without any newly added signatures).
  • If anything changes in the signature type's array, that signature type is removed. For example, if a transaction is signed with two secret key signatures, then one signature is removed or updated, the secret key signature will be removed from the transaction (other types are not affected).
  • On every change in signature type arrays, the transaction is signed by going through all signature type arrays and adding signatures that are currently in the arrays. This way, we're guaranteed always to have the most up-to-date list of signatures. The performance will be fine because we can add 20 signatures to a transaction at most.
  • This approach should scale well if we need to add more signature types in the future.
  • The current UI allows only a single hardware wallet or extension wallet signature. We can support adding multiple if needed.

@quietbits quietbits marked this pull request as draft October 10, 2024 22:48
@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

@quietbits quietbits marked this pull request as ready for review October 11, 2024 14:14
@quietbits quietbits linked an issue Oct 11, 2024 that may be closed by this pull request
@quietbits
Copy link
Contributor Author

@stellar/security-team, could you please do a sanity check on this Lab PR? 🙏

Copy link
Contributor

@aristidesstaffieri aristidesstaffieri left a comment

Choose a reason for hiding this comment

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

lgtm. I don't think I have the context to know much about the impact of the MultiPicker changes but the signature helpers and functionality look good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog (Not Ready)
Development

Successfully merging this pull request may close these issues.

Problem: Sign buttons and UI ambiguous
3 participants