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

Rework client.TxBuilder #8138

Open
4 tasks
robert-zaremba opened this issue Dec 10, 2020 · 4 comments
Open
4 tasks

Rework client.TxBuilder #8138

robert-zaremba opened this issue Dec 10, 2020 · 4 comments
Labels
C:Crypto S:needs architecture Needs a architecture proposal for how the feature will be implemented T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T: Client UX T: Dev UX UX for SDK developers (i.e. how to call our code)

Comments

@robert-zaremba
Copy link
Collaborator

Summary

client.TxBuilder and it's implementation (x/auth/tx.wrapper) are badly implemented:

  • We can't easily observe a state of TxBuilder
  • Client signing (client/tx.Sign) functionality is twisted and requires a use of external object (txFactory) which may be in conflict with txBuilder state.
  • Using builder and factory at the same time is error prone.

Context:

Proposal

Usage of txBuilder should be investigated.
Redesign client.TxBuilder interface and reimplement tx.wrapper .


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@robert-zaremba robert-zaremba added T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). C:Crypto T: Dev UX UX for SDK developers (i.e. how to call our code) T: Client UX labels Dec 10, 2020
@robert-zaremba robert-zaremba added this to the v0.41 milestone Dec 10, 2020
@robert-zaremba
Copy link
Collaborator Author

robert-zaremba commented Dec 11, 2020

Copying a conversation related to the signing issue:

We have a problem with off-chain transaction signing in the DIRECT mode.

In #8106 (fix: Signature only flag bug on tx sign command) we had a lengthy discussion about expected use case of CLI signing. The main user story is:

  1. someone generates the transaction and send it to the users
  2. users sign it
  3. someone collects the signatures, put them in a transaction and send to a blockchain.

A variant of the above is a sequential process, where users sign and pass from one to another the transaction.

This doesn't work after launchpad. Few reasons:

  • Refactore of tx.Sign - the function assumes a single signer only. We changed it this week to allow aggregation of signatures, but it won't work on multiple users signing in a direct mode.
  • current implementation of Sign_mode_direct: it requires each user to sign any other user public key and singing mode. Moreover, the signing mode has to be the same for all users and exactly the same as the one set on the chain (AnteHandler.SigVerificationDecorator uses it's own signing mode variable).
  • Signing order is defined during transaction creation and it can't be altered unless we change the implementation of AnteHandler.SigVerificationDecorator and review other places where we relay on that order.

We made it working for a single signature use case, but for multiple signatures the process is more constrained as explained above.

@aaronc
Copy link
Member

aaronc commented Mar 4, 2021

Let's make sure we address ADR 031 TxBuilder UX (see #8728 (comment))

@clevinson
Copy link
Contributor

As part of this we should also be looking at whatever is leftover from #7357

@educlerici-zondax educlerici-zondax added the S:zondax Squad: Zondax label Oct 9, 2023
@tac0turtle tac0turtle removed this from the Feature Backlog milestone Nov 13, 2023
@tac0turtle tac0turtle added S:needs architecture Needs a architecture proposal for how the feature will be implemented and removed T:Sprint S:zondax Squad: Zondax labels Nov 14, 2023
@raynaudoe raynaudoe mentioned this issue Apr 17, 2024
12 tasks
@educlerici-zondax
Copy link
Contributor

this will be on hold until the implementation of ADR071 is completed.
After that, we can assess the possibility of applying these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Crypto S:needs architecture Needs a architecture proposal for how the feature will be implemented T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T: Client UX T: Dev UX UX for SDK developers (i.e. how to call our code)
Projects
Status: ☃️ Icebox
Development

No branches or pull requests

6 participants