feat: Add AuxTxBuilder #10455

amaury1093 · 2021-10-28T14:26:48Z

Description

For creating an intermediary/auxiliary tx (e.g. by the tipper in tipped transactions), using the existing client.TxBuilder is awkward. We propose a new client-side builder for this purpose.

API Usage (e.g. how the tipper would programmtically use this):

// Note: there's no need to use clientCtx.TxConfig anymore!

bldr := clienttx.NewAuxTxBuilder()
err := bldr.SetMsgs(msgs...)
bldr.SetAddress("cosmos1...")
bldr.SetMemo(...)
bldr.SetTip(...)
bldr.SetPubKey(...)
err := bldr.SetSignMode(...) // DIRECT_AUX or AMINO, or else error
// ... other setters are available

// Get the bytes to sign.
signBz, err := bldr.GetSignBytes()

// Sign the bz using your favorite method.
sig, err := privKey.sign(signBz)

// Set the signature
bldr.SetSig(sig)

// Get the final auxSignerData to be sent to the fee payer
auxSignerData, err:= bldr.GetAuxSignerData()

auxSignerData is a protobuf message, whose JSON reprensentation looks like:

{
  "address": "cosmos1...",
  "mode": "SIGN_MODE_{DIRECT_AUX,LEGACY_AMINO_JSON}",
  "sign_doc": {
    "body_bytes": "{base64 bytes}",
    "public_key": {
      "@type": "cosmos.sepc256k1.PubKey",
      "key": "{base64 bytes}"
    },
    "chain_id": "...",
    "account_number": 24,
    "sequence": 42,
    "tip": {
      "amount": [{ "denom": "uregen", "amount": 1000 }],
      "tipper": "cosmos1..."
    }
  },
  "sig": "{base64 bytes}"
}

Then the fee payer would use the TxBuilder to construct the final TX, with a new helper method: AddAuxSignerData:

// get auxSignerData from AuxTxBuilder
auxSignerData := ...

txBuilder := txConfig.NewTxBuilder()
err := txBuilder.AddAuxSignerData(auxSignerData)

// Set fee payer data
txBuilder.SetFee()
txBuilder.SetGasLimit()
txBuilder.SetFeePayer()

sigs, err := txBuilder.GetSignaturesV2()
auxSig := sigs[0] // the aux signer's signature

// Set all signer infos (1st round of calling SetSignatures)
txBuilder.SetSignatures(
  auxSig,                   // The aux SignatureV2
  signing.SignatureV2{...}, // The feePayer's SignatureV2
)

// Sign
signBz, err = encCfg.TxConfig.SignModeHandler().GetSignBytes(...)
feepayerSig, err := feepayerPriv.Sign(signBz)

// Set all signatures (2nd round of calling SetSignatures)
txBuilder.SetSignatures(
  auxSig,                   // The aux SignatureV2
  signing.SignatureV2{feepayerSig, ...}, // The feePayer's SignatureV2
)

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

client/tx/aux_builder.go

codecov · 2021-10-28T14:43:00Z

Codecov Report

Merging #10455 (2c68763) into master (2394266) will decrease coverage by 0.23%.
The diff coverage is 0.00%.

❗ Current head 2c68763 differs from pull request most recent head 825b286. Consider uploading reports for the commit 825b286 to get more accurate results

@@            Coverage Diff             @@
##           master   #10455      +/-   ##
==========================================
- Coverage   64.33%   64.09%   -0.24%     
==========================================
  Files         592      582      -10     
  Lines       56008    55299     -709     
==========================================
- Hits        36034    35446     -588     
+ Misses      17912    17835      -77     
+ Partials     2062     2018      -44

Impacted Files	Coverage Δ
client/tx/aux_builder.go	`0.00% <0.00%> (ø)`
x/distribution/simulation/operations.go	`80.54% <0.00%> (-9.73%)`	⬇️
x/nft/keeper/class.go	`73.33% <0.00%> (-6.67%)`	⬇️
x/mint/module.go	`52.54% <0.00%> (-3.02%)`	⬇️
store/cachekv/store.go	`77.63% <0.00%> (-1.32%)`	⬇️
x/mint/types/genesis.go	`0.00% <0.00%> (ø)`
crypto/keys/secp256k1/secp256k1_nocgo.go	`83.33% <0.00%> (ø)`
cosmovisor/cmd/cosmovisor/cmd/root.go
cosmovisor/cmd/cosmovisor/cmd/help.go
cosmovisor/cmd/cosmovisor/cmd/run.go
... and 9 more

fdymylja · 2021-10-28T15:36:36Z

God bless this API @AmauryM! How does the API look from the tx fee signer PoV? Assuming that I am the tipper, I get my tx bytes, now I forward them to someone who wants to pay the fees for me, how does the API look using aux tx builder?

amaury1093 · 2021-10-28T15:48:00Z

How does the API look from the tx fee signer PoV?

The fee signer will use DIRECT (or LEGACY_AMINO_JSON maybe), so he will use the existing client.TxBuilder (aka the wrapper).

Probably a good idea is to add a func (w *wrapper) SetAuxSignerData(asd tx.AuxSignerData) helper on client.TxBuilder, for the fee payer. I'll still need to see how this plays with its SetSignatures

aaronc · 2021-10-28T16:06:56Z

How does the API look from the tx fee signer PoV?

The fee signer will use DIRECT (or LEGACY_AMINO_JSON maybe), so he will use the existing client.TxBuilder (aka the wrapper).

Probably a good idea is to add a func (w *wrapper) SetAuxSignerData(asd tx.AuxSignerData) helper on client.TxBuilder, for the fee payer. I'll still need to see how this plays with its SetSignatures

Probably AddAuxSignerData.

Would be great to find a way to simplify the signing workflow too. How does this interact with pubkeys and sign modes?

…0443-aux-builder

proto/cosmos/tx/v1beta1/tx.proto

amaury1093 · 2021-11-08T18:11:20Z

x/auth/tx/aux_test.go

+	// Fee payer creates a TxBuilder.
+	w := encCfg.TxConfig.NewTxBuilder()
+	err = w.AddAuxSignerData(auxSignerData)
+	require.NoError(t, err)
+	w.SetFeePayer(feepayerAddr)
+	w.SetFeeAmount(fee)
+	w.SetGasLimit(gas)
+	sigs, err := w.(authsigning.SigVerifiableTx).GetSignaturesV2()
+	require.NoError(t, err)
+	tipperSigV2 := sigs[0]
+	// Set all signer infos.
+	w.SetSignatures(tipperSigV2, signing.SignatureV2{
+		PubKey:   feepayerPk,
+		Sequence: 15,
+	})
+	signBz, err = encCfg.TxConfig.SignModeHandler().GetSignBytes(
+		signing.SignMode_SIGN_MODE_DIRECT,
+		authsigning.SignerData{Address: feepayerAddr.String(), ChainID: chainID, AccountNumber: 11, Sequence: 15, SignerIndex: 1},
+		w.GetTx(),
+	)
+	require.NoError(t, err)
+	feepayerSig, err := feepayerPriv.Sign(signBz)
+	require.NoError(t, err)
+	// Set all signatures.
+	w.SetSignatures(tipperSigV2, signing.SignatureV2{
+		PubKey: feepayerPk,
+		Data: &signing.SingleSignatureData{
+			SignMode:  signing.SignMode_SIGN_MODE_DIRECT,
+			Signature: feepayerSig,
+		},
+		Sequence: 15,
+	})


Fee payer POV using TxBuilder. It's still calling SetSignatures twice.

amaury1093 · 2021-11-08T18:12:03Z

Marking as R4R. @fdymylja @aaronc I also added in OP the fee payer's TxBuilder DX.

aaronc

Looks pretty close but left one comment to address

aaronc · 2021-11-08T18:46:43Z

x/auth/tx/builder.go

+	w.bodyBz = data.SignDoc.BodyBytes
+	w.SetTip(data.GetSignDoc().GetTip())
+
+	w.setSignerInfos(append(w.tx.AuthInfo.SignerInfos, &tx.SignerInfo{


What if this is the wrong order? This method should check the signers and insert it in the correct position I think

I added this in b9f3cf3. It required 2 changes:

adding the interface registry to the wrapper, because it's needed to decode data.SignDoc.BodyBytes to get the Msgs.GetSigners()

each AuxSignerData has the signer's PubKey, but not address. We'll either need to use a bech32 converter (aka for now: the global sdk.AccAddress), or maybe we can add a new address field in AuxSignerData

I'll try adding a new address field in AuxSignerData

Edit: done in 0917b76

amaury1093 · 2021-11-09T15:56:26Z

proto/cosmos/tx/v1beta1/tx.proto

+	// sign_doc is the SIGN_MOD_DIRECT_AUX sign doc that the auxiliary signer
+  // signs. Note: we use the same sign doc even if we're signing with
+  // LEGACY_AMINO_JSON.
+	SignDocDirectAux sign_doc = 2;


The Tx body_bytes is already in SignDocDirectAux. However, do we also want to copy it here as a top-level TxBody body = 3; field?

Pros:

If using JSON, then the fee payer can have human readable description of msgs, without needing to deserialize the body_bytes. Currently, we only see base64 bytes.

Cons:

More data to be transmitted over the wire (as it's a duplicate of sign_doc.body_bytes)

only useful for JSON to be read by humans. In general, for machine-to-machine, proto binary should be preferred (more compact)

amaury1093 · 2021-11-09T16:15:24Z

proto/cosmos/tx/v1beta1/tx.proto

+  // LEGACY_AMINO_JSON.
+	SignDocDirectAux sign_doc = 2;
+  // mode is the signing mode of the single signer
+  cosmos.tx.signing.v1beta1.SignMode mode = 3;


One thing I thought about looking at this: what if the aux signer is a legacy multisig? Do we want to support that?

If yes we should probably do:

Suggested change

cosmos.tx.signing.v1beta1.SignMode mode = 3;

cosmos.tx.signing.v1beta1.ModeInfo mode_info = 3;

How hard would it be to add this? I can't imagine multisigs using tips but there may be cases where it makes sense for them to do aux signing

Say we have a 2/2 multisig as tipper. The 2 signers will use AuxTxBuilder, and use a single mode info anyways.

Who should aggregate the signatures? Probably the fee payer I'd say? In which case I believe it's actually better API to keep cosmos.tx.signing.v1beta1.SignMode mode = 3; here, and maybe add a AddMultisigAuxSignerData on the fee payer's txbuilder

i-norden

Added some comments and questions!

client/tx/aux_builder.go

i-norden · 2021-11-09T18:17:46Z

client/tx/aux_builder.go

+// GetSignBytes returns the builder's sign bytes.
+func (b *AuxTxBuilder) GetSignBytes() ([]byte, error) {
+	auxTx := b.auxSignerData
+	if auxTx == nil {


Should we add additional/deeper checks in these initial guards? A call to a single setter that calls checkEmptyFields() will make b.body, b.auxSignerData, and b.auxSignerData.SignDoc non-nil but that doesn't mean they are properly set.

The way I was thinking was to add some nil-guards here. Then, for actual validation, we call b.auxSignerData.SignDoc.ValidateBasic() and b.auxSignerData.ValidateBasic().

client/tx/aux_builder.go

client/tx/aux_builder_test.go

types/tx/aux_test.go

i-norden · 2021-11-09T20:41:40Z

x/auth/tx/aux_test.go

+	tipperBuilder.SetAddress(tipperAddr.String())
+	tipperBuilder.SetAccountNumber(1)
+	tipperBuilder.SetSequence(2)
+	tipperBuilder.SetTimeoutHeight(3)


Possibly a stupid question but what would happen if the tipper and the aux signer had different timeout heights? As far as I can tell this wouldn't throw any errors anywhere in this building process, and the final tx timeout will be whichever timeout is on the last signer data fed to AddAuxSignerData(). Actually I think this question extends to all the signer data that is not being set at a specific index in AddAuxSignerData().

not a stupid question! I thought about this too, but I went for the lazy way, which is that signature verification will fail when sending this tx (with 2 different timeout heights) to the node.

But maybe it's better to catch this early. I'll add some basic checks in AddAuxSignerData

aaronc

LGTM. It may make sense to support multisigs, however. If it's not too hard I would vote yes.

i-norden

LGTM!

@type

## Description Closes: cosmos#10443 For creating an intermediary/auxiliary tx (e.g. by the tipper in tipped transactions), using the existing `client.TxBuilder` is awkward. We propose a new client-side builder for this purpose. API Usage (e.g. how the tipper would programmtically use this): ```go // Note: there's no need to use clientCtx.TxConfig anymore! bldr := clienttx.NewAuxTxBuilder() err := bldr.SetMsgs(msgs...) bldr.SetAddress("cosmos1...") bldr.SetMemo(...) bldr.SetTip(...) bldr.SetPubKey(...) err := bldr.SetSignMode(...) // DIRECT_AUX or AMINO, or else error // ... other setters are available // Get the bytes to sign. signBz, err := bldr.GetSignBytes() // Sign the bz using your favorite method. sig, err := privKey.sign(signBz) // Set the signature bldr.SetSig(sig) // Get the final auxSignerData to be sent to the fee payer auxSignerData, err:= bldr.GetAuxSignerData() ``` auxSignerData is a protobuf message, whose JSON reprensentation looks like: ```json { "address": "cosmos1...", "mode": "SIGN_MODE_{DIRECT_AUX,LEGACY_AMINO_JSON}", "sign_doc": { "body_bytes": "{base64 bytes}", "public_key": { "@type": "cosmos.sepc256k1.PubKey", "key": "{base64 bytes}" }, "chain_id": "...", "account_number": 24, "sequence": 42, "tip": { "amount": [{ "denom": "uregen", "amount": 1000 }], "tipper": "cosmos1..." } }, "sig": "{base64 bytes}" } ``` Then the fee payer would use the TxBuilder to construct the final TX, with a new helper method: `AddAuxSignerData`: ```go // get auxSignerData from AuxTxBuilder auxSignerData := ... txBuilder := txConfig.NewTxBuilder() err := txBuilder.AddAuxSignerData(auxSignerData) // Set fee payer data txBuilder.SetFee() txBuilder.SetGasLimit() txBuilder.SetFeePayer() sigs, err := txBuilder.GetSignaturesV2() auxSig := sigs[0] // the aux signer's signature // Set all signer infos (1st round of calling SetSignatures) txBuilder.SetSignatures( auxSig, // The aux SignatureV2 signing.SignatureV2{...}, // The feePayer's SignatureV2 ) // Sign signBz, err = encCfg.TxConfig.SignModeHandler().GetSignBytes(...) feepayerSig, err := feepayerPriv.Sign(signBz) // Set all signatures (2nd round of calling SetSignatures) txBuilder.SetSignatures( auxSig, // The aux SignatureV2 signing.SignatureV2{feepayerSig, ...}, // The feePayer's SignatureV2 ) ``` --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

amaury1093 added 2 commits October 28, 2021 15:40

create auxtx

fd811cc

Add setters on AuxTxBuilder

e7c1f70

github-actions bot added the C:CLI label Oct 28, 2021

Add set tip

2c68763

amaury1093 commented Oct 28, 2021

View reviewed changes

client/tx/aux_builder.go Outdated Show resolved Hide resolved

amaury1093 requested review from aaronc, gsora and atheeshp October 28, 2021 14:34

Rename to AuxSignerData

dcfe5ae

amaury1093 changed the title ~~feat: Add AuxTx and AuxTxBuilder~~ feat: Add AuxTxBuilder Oct 28, 2021

amaury1093 added 5 commits November 2, 2021 18:40

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am/1…

d2f9ac4

…0443-aux-builder

Add sign mode in AuxSignerData

fee39db

wip

36ca5a7

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am/1…

4f73b55

…0443-aux-builder

Add tests

72333b3

github-actions bot added the C:x/auth label Nov 8, 2021

amaury1093 commented Nov 8, 2021

View reviewed changes

proto/cosmos/tx/v1beta1/tx.proto Show resolved Hide resolved

amaury1093 added 2 commits November 8, 2021 16:13

txBuilder AddAuxSignerData

cbba311

Add TestBuilderWithAux

a249828

amaury1093 marked this pull request as ready for review November 8, 2021 15:59

amaury1093 requested a review from alexanderbez as a code owner November 8, 2021 15:59

amaury1093 added 3 commits November 8, 2021 18:55

Remove AddSignature

eaf32d3

Add since

0a04112

make proto-gen

9d18d47

amaury1093 commented Nov 8, 2021

View reviewed changes

aaronc reviewed Nov 8, 2021

View reviewed changes

amaury1093 added 5 commits November 9, 2021 12:24

AddAuxSignerData correct order

b9f3cf3

Add addess in AuxSignerData

0917b76

Merge branch 'master' into am/10443-aux-builder

8a76a75

Fix proto lint

0acd018

Merge branch 'master' into am/10443-aux-builder

3d95459

amaury1093 assigned atheeshp and aaronc Nov 9, 2021

i-norden self-assigned this Nov 9, 2021

amaury1093 commented Nov 9, 2021

View reviewed changes

i-norden reviewed Nov 9, 2021

View reviewed changes

amaury1093 added 2 commits November 10, 2021 11:16

Address reviews

0d752ef

Remove tip.amount == nil validation

5327a79

aaronc approved these changes Nov 10, 2021

View reviewed changes

i-norden approved these changes Nov 10, 2021

View reviewed changes

tac0turtle added the A:automerge Automatically merge PR once all prerequisites pass. label Nov 11, 2021

tac0turtle added 2 commits November 11, 2021 11:01

Merge branch 'master' into am/10443-aux-builder

77ab596

Merge branch 'master' into am/10443-aux-builder

825b286

mergify bot merged commit 8fc9f76 into master Nov 11, 2021

mergify bot deleted the am/10443-aux-builder branch November 11, 2021 11:25

This was referenced Nov 15, 2021

fix: AuxTxBuilder check all AuxSignerData match #10547

Merged

feat: Add Tips middleware #10208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add AuxTxBuilder #10455

feat: Add AuxTxBuilder #10455

amaury1093 commented Oct 28, 2021 •

edited

Loading

codecov bot commented Oct 28, 2021 •

edited

Loading

fdymylja commented Oct 28, 2021

amaury1093 commented Oct 28, 2021

aaronc commented Oct 28, 2021

amaury1093 Nov 8, 2021

amaury1093 commented Nov 8, 2021

aaronc left a comment

aaronc Nov 8, 2021

amaury1093 Nov 9, 2021

amaury1093 Nov 9, 2021 •

edited

Loading

amaury1093 Nov 9, 2021 •

edited

Loading

amaury1093 Nov 9, 2021

aaronc Nov 10, 2021

amaury1093 Nov 15, 2021

i-norden left a comment

i-norden Nov 9, 2021

amaury1093 Nov 10, 2021 •

edited

Loading

i-norden Nov 9, 2021

amaury1093 Nov 10, 2021

aaronc left a comment

i-norden left a comment

	cosmos.tx.signing.v1beta1.SignMode mode = 3;
	cosmos.tx.signing.v1beta1.ModeInfo mode_info = 3;

feat: Add AuxTxBuilder #10455

feat: Add AuxTxBuilder #10455

Conversation

amaury1093 commented Oct 28, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Oct 28, 2021 • edited Loading

Codecov Report

fdymylja commented Oct 28, 2021

amaury1093 commented Oct 28, 2021

aaronc commented Oct 28, 2021

Choose a reason for hiding this comment

amaury1093 commented Nov 8, 2021

aaronc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

amaury1093 Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i-norden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc left a comment

Choose a reason for hiding this comment

i-norden left a comment

Choose a reason for hiding this comment

amaury1093 commented Oct 28, 2021 •

edited

Loading

codecov bot commented Oct 28, 2021 •

edited

Loading

amaury1093 Nov 9, 2021 •

edited

Loading

amaury1093 Nov 9, 2021 •

edited

Loading

amaury1093 Nov 10, 2021 •

edited

Loading