feat(x/auth): batch transactions file format & broadcast multi transactions #18281
Conversation
WalkthroughThe Cosmos SDK has introduced a feature that allows for the broadcasting of multiple transactions at once. This is achieved through updates to various components, including transaction encoding/decoding interfaces, CLI commands for broadcasting and signing transactions, and the addition of a new protobuf message to wrap multiple transactions. The changes enhance the flexibility and efficiency of transaction processing within the Cosmos SDK ecosystem. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 11
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- invalid_batch.json
- new_batch.json
- types/tx/tx.pb.go
Files selected for processing (10)
- api/cosmos/tx/v1beta1/tx.pulsar.go (54} hunks)
- client/tx_config.go (1} hunks)
- proto/cosmos/tx/v1beta1/tx.proto (1} hunks)
- types/tx_msg.go (1} hunks)
- x/auth/client/cli/broadcast.go (2} hunks)
- x/auth/client/cli/tx_sign.go (4} hunks)
- x/auth/client/tx.go (1} hunks)
- x/auth/tx/config.go (5} hunks)
- x/auth/tx/decoder.go (1} hunks)
- x/auth/tx/encoder.go (1} hunks)
Files not summarized due to errors (1)
- api/cosmos/tx/v1beta1/tx.pulsar.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- proto/cosmos/tx/v1beta1/tx.proto
Additional comments: 25
client/tx_config.go (1)
- 20-22: The new methods
TxsJSONEncoder(),TxsJSONDecoder()are added to theTxEncodingConfiginterface to handle the encoding and decoding of multiple transactions (Txs). Ensure that all implementations of this interface throughout the codebase have been updated to include these new methods. Also, verify that these methods are correctly used where multiple transactions need to be processed.x/auth/client/tx.go (2)
- 118-134: The new function
ReadTxsFromFileis introduced to read and decode multiple transactions from a file. This function is similar to the existingReadTxFromFilefunction, but it uses theTxsJSONDecodermethod instead ofTxJSONDecoderto handle multiple transactions. This is a good addition as it allows for batch processing of transactions, improving efficiency. However, error handling could be improved. Currently, if an error occurs while reading the file, the function simply returns without providing any information about the error. It would be better to wrap the error with a meaningful message before returning it. This would make it easier to debug issues if they occur. Here's a suggested change:* 135-135: The comment for the `ReadTxsFromInput` function is not updated to reflect the changes in the function. It still says that the function does not decode the transactions, which is no longer true. Please update the comment to accurately describe the function's behavior. </blockquote></details> <details><summary>x/auth/tx/config.go (5)</summary><blockquote> * 25-26: The addition of `txsJsonDecoder` and `txsJsonEncoder` to the `config` struct is a good move. It allows for the handling of multiple transactions (`Txs`) in addition to single transactions (`Tx`). This change enhances the flexibility of the transaction handling system. * 59-62: The addition of `TxsJSONDecoder` and `TxsJSONEncoder` to the `ConfigOptions` struct is consistent with the changes made to the `config` struct. These new fields allow for the configuration of the encoding and decoding of multiple transactions (`Txs`). * 175-181: The `NewTxConfigWithOptions` function has been updated to initialize the new `txsJsonDecoder` and `txsJsonEncoder` fields of the `config` struct. This is a necessary change to support the handling of multiple transactions (`Txs`). * 195-200: The `NewTxConfigWithOptions` function now also checks if `TxsJSONDecoder` and `TxsJSONEncoder` are provided in the `configOptions`. If not, it sets them to default values. This is a good practice as it ensures that the `config` struct is always fully initialized. * 270-277: The addition of `TxsJSONDecoder` and `TxsJSONEncoder` methods to the `config` struct allows for the retrieval of the `txsJsonDecoder` and `txsJsonEncoder` fields. This is a necessary change to support the handling of multiple transactions (`Txs`). </blockquote></details> <details><summary>api/cosmos/tx/v1beta1/tx.pulsar.go (17)</summary><blockquote> * 660-664: The `_Txs_1_list` struct is a wrapper around a slice of pointers to `Tx` objects. This is used to implement the `protoreflect.List` interface for the `Txs` field in the `Txs` message. This is a common pattern in Go when you want to add methods to a type, in this case a slice of `Tx` pointers. * 666-671: The `Len` method is part of the `protoreflect.List` interface. It returns the length of the list. If the list is `nil`, it returns 0. This is a standard implementation of the `Len` method for a slice. * 673-675: The `Get` method is part of the `protoreflect.List` interface. It returns the `i`th element of the list as a `protoreflect.Value`. This is a standard implementation of the `Get` method for a slice. * 677-681: The `Set` method is part of the `protoreflect.List` interface. It sets the `i`th element of the list to `value`. The value is unwrapped and cast to a `*Tx` before being set. This is a standard implementation of the `Set` method for a slice. * 683-687: The `Append` method is part of the `protoreflect.List` interface. It appends `value` to the end of the list. The value is unwrapped and cast to a `*Tx` before being appended. This is a standard implementation of the `Append` method for a slice. * 689-693: The `AppendMutable` method is part of the `protoreflect.List` interface. It appends a new `Tx` to the end of the list and returns it as a `protoreflect.Value`. This is a standard implementation of the `AppendMutable` method for a slice. * 695-700: The `Truncate` method is part of the `protoreflect.List` interface. It truncates the list to `n` elements. All elements after the `n`th are set to `nil` and removed. This is a standard implementation of the `Truncate` method for a slice. * 702-705: The `NewElement` method is part of the `protoreflect.List` interface. It returns a new `Tx` as a `protoreflect.Value`. This is a standard implementation of the `NewElement` method for a slice. * 707-709: The `IsValid` method is part of the `protoreflect.List` interface. It returns `true` if the list is not `nil`. This is a standard implementation of the `IsValid` method for a slice. * 716-720: The `init` function initializes the `md_Txs` and `fd_Txs_txs` variables with the message descriptor for the `Txs` message and the field descriptor for the `txs` field in the `Txs` message, respectively. This is a common pattern in Go for initializing global variables. * 726-728: The `ProtoReflect` method is part of the `protoreflect.Message` interface. It returns the receiver as a `protoreflect.Message`. This is a standard implementation of the `ProtoReflect` method for a message. * 730-740: The `slowProtoReflect` method is a fallback implementation of the `ProtoReflect` method. It is used when the `protoimpl.UnsafeEnabled` flag is `false` or the receiver is `nil`. This is a standard implementation of the `slowProtoReflect` method for a message. * 742-746: The `fastReflection_Txs_messageType` struct is used to implement the `protoreflect.MessageType` interface for the `Txs` message. This is a common pattern in Go when you want to add methods to a type, in this case the `Txs` message. * 748-752: The `New` method is part of the `protoreflect.MessageType` interface. It returns a new `fastReflection_Txs` as a `protoreflect.Message`. This is a standard implementation of the `New` method for a message type. * 754-755: The `Descriptor` method is part of the `protoreflect.MessageType` interface. It returns the message descriptor for the `Txs` message. This is a standard implementation of the `Descriptor` method for a message type. * 759-761: The `Descriptor` method is part of the `protoreflect.Message` interface. It returns the message descriptor for the `Txs` message. This is a standard implementation of the `Descriptor` method for a message. * 766-768: The `Type` method is part of the `protoreflect.Message` interface. It returns the message type for the `Txs` message. This is a standard implementation of the `Type` method for a message. </blockquote></details></blockquote></details> </details>
![github-advanced-security[bot]](https://github.com/avatars/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (4)
- x/auth/client/cli/broadcast.go (2} hunks)
- x/auth/client/cli/tx_sign.go (4} hunks)
- x/auth/client/tx.go (1} hunks)
- x/auth/tx/decoder.go (1} hunks)
Additional comments: 4
x/auth/client/tx.go (1)
- 118-134: The function
ReadTxsFromFilereads and decodes multiple transactions from a file or from stdin. However, the error handling is not sufficient. If an error occurs while reading the file or stdin, the function simply returns without providing any context about the error. This could make debugging difficult if an error occurs. It would be better to wrap the error with a message that provides more context. Here's how you can improve the error handling:</blockquote></details> <details><summary>x/auth/client/cli/broadcast.go (1)</summary><blockquote> * 51-65: The code correctly handles the broadcasting of multiple transactions. It loops over the transactions, encodes each one, broadcasts it, and prints the result. This is a good approach for handling multiple transactions. </blockquote></details> <details><summary>x/auth/client/cli/tx_sign.go (2)</summary><blockquote> * 172-172: The `signingTxs` slice is still being initialized with a length of 0. If the number of transactions is known beforehand, it would be more efficient to initialize it with that capacity. If the number of transactions is not known, this is fine. * 440-454: The `marshalBatchJSON` function has been added to handle the marshalling of multiple transactions into JSON. It checks if only the signature is needed and if so, it retrieves the signatures from the first transaction and marshals them into JSON. If the full transactions are needed, it creates a new slice of `sdk.Tx` and appends each transaction to it before marshalling the entire slice into JSON. This function seems to handle both cases correctly. However, it's worth noting that if `signatureOnly` is true, only the signatures from the first transaction are returned. If the signatures from all transactions are needed, this function would need to be modified. </blockquote></details></blockquote></details> </details>
julienrbrt
changed the title
invalid_batch.json
Outdated
There was a problem hiding this comment.
can we not store test data in the root?
There was a problem hiding this comment.
Moved to the test_samples folder, also I think we will remove it in the future
|
The bot had some good comments. Could you apply the relevant ones? |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- x/auth/client/cli/tx_sign.go (4 hunks)
- x/auth/client/tx.go (1 hunks)
- x/auth/tx/decoder.go (1 hunks)
- x/auth/tx/encoder.go (1 hunks)
Additional comments: 6
x/auth/tx/decoder.go (1)
- 94-117: The new function
DefaultJSONTxsDecoderis introduced to decode multiple transactions from a byte slice. It seems to be correctly implemented, but it's important to ensure that thesdk.Txobjects are correctly wrapped in thewrapperstruct. Also, make sure that theProtoCodecMarshalerpassed to this function is capable of correctly unmarshaling the JSON intotx.Txsobjects.x/auth/tx/encoder.go (1)
- 43-56: The new function
DefaultJSONTxsEncoderseems to be well implemented. It correctly wraps each transaction in thetxsslice and appends it to thewrappedTxs.Txsslice. The error handling for type assertion is also correctly done. However, it's important to ensure that thecdc.MarshalJSONfunction can handle the&wrappedTxsinput correctly and that the output is as expected. Also, ensure that thesdk.TxsEncodertype returned by this function is correctly used in the rest of the codebase.x/auth/client/cli/tx_sign.go (4)
12-16: The import of the package "github.com/cosmos/cosmos-sdk/x/auth/signing" is a new addition. Ensure that it is used appropriately in the code.
172-172: The
signingTxsslice is initialized with a length of 0. It's more efficient to initialize it with a capacity if the number of transactions is known beforehand. If the number of transactions is not known, this is fine.191-200: The
signingTxsslice is populated with signed transactions and then marshalled into JSON format using themarshalBatchJSONfunction. Ensure that themarshalBatchJSONfunction is correctly implemented and handles any potential errors during the marshalling process.436-454: The
marshalBatchJSONfunction is introduced to marshal a batch of transactions into JSON format. It checks if thesignatureOnlyflag is set and if so, it only marshals the signatures. Otherwise, it marshals the entire transactions. Ensure that the function correctly handles any potential errors during the marshalling process.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- tests/e2e/auth/suite.go (2 hunks)
- x/auth/client/cli/tx_sign.go (4 hunks)
Files skipped from review due to trivial changes (1)
- tests/e2e/auth/suite.go
Additional comments: 4
x/auth/client/cli/tx_sign.go (4)
12-16: The import statement for the "signing" package is added. Ensure that this package is available in the project dependencies.
194-194: Each
txBuilder.GetTx()is appended to thesigningTxsslice. Ensure thattxBuilder.GetTx()returns a validsigning.Txobject.196-200: The
marshalBatchJSONfunction is called with thetxCfg,signingTxs, andprintSignatureOnlyarguments. Ensure that these arguments are valid and that the function handles any potential errors.440-466: The
marshalBatchJSONfunction is added, which takes thetxConfig,txs, andsignatureOnlyparameters and returns a JSON-encoded byte array. This function seems to handle both cases wheresignatureOnlyis true or false. Ensure that the function correctly handles these cases and that the returned byte array is correctly formatted.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- test_samples/invalid_batch.json
- test_samples/new_batch.json
Files selected for processing (1)
- tests/e2e/auth/suite.go (2 hunks)
Files skipped from review due to trivial changes (1)
- tests/e2e/auth/suite.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/auth/client/cli/broadcast.go (1 hunks)
- x/auth/client/tx.go (1 hunks)
Additional comments: 2
x/auth/client/tx.go (2)
115-116: The function
ReadTxFromFilereads a single transaction from a file. It's good that you're checking for errors when reading the file and returning the error if any.151-153: The function
ReadTxsFromInputreads multiple transactions from the given filenames but does not decode the transactions. It's good that you're checking for errors when reading the file and returning a meaningful error message. However, it would be better to also handle the case where no filenames are provided. This can be done by checking iffilenamesis empty at the start of the function. If it is, return an error indicating that no filenames were provided.- if len(filenames) == 0 { - return nil, fmt.Errorf("no file name provided") - }(The previous comment made by me in a comment chain is still valid)
julienrbrt
changed the title
| @@ -115,6 +115,39 @@ func ReadTxFromFile(ctx client.Context, filename string) (tx sdk.Tx, err error) | |||
| return ctx.TxConfig.TxJSONDecoder()(bytes) | |||
| } | |||
|
|
|||
| // Read and decode a multi transactions (must be in Txs format) from the given filename. | |||
| // Can pass "-" to read from stdin. | |||
| func ReadTxsFromFile(ctx client.Context, filename string) (tx []sdk.Tx, err error) { | |||
There was a problem hiding this comment.
Can you add a test for this?
CHANGELOG.md
Outdated
| @@ -52,6 +52,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
| * (server) [#17094](https://github.com/cosmos/cosmos-sdk/pull/17094) Add duration `shutdown-grace` for resource clean up (closing database handles) before exit. | |||
| * (x/auth/vesting) [#17810](https://github.com/cosmos/cosmos-sdk/pull/17810) Add the ability to specify a start time for continuous vesting accounts. | |||
| * (baseapp) [#18071](https://github.com/cosmos/cosmos-sdk/pull/18071) Add hybrid handlers to `MsgServiceRouter`. | |||
| * (x/auth) [#18281](https://github.com/cosmos/cosmos-sdk/pull/18281) Allow broadcast multi txs. | |||
There was a problem hiding this comment.
nit, can you move the changelog to x/auth/CHANGELOG.md?
There was a problem hiding this comment.
auth haven't have a changelog yet so I created one.
There was a problem hiding this comment.
It does: https://github.com/cosmos/cosmos-sdk/blob/main/x/auth/CHANGELOG.md. Maybe rebase?
x/auth/client/cli/broadcast.go
Outdated
| // Get transactions from input file | ||
| // Work for both only single tx or batch tx |
There was a problem hiding this comment.
IMO this comment is superfluous. Consider removing or refactoring.
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
|
seems like there is an integration test failing, any chance this could be fixed? |
Cause I dont have push permission anymore on this branch anymore, I will close this pr and open other one. |
Description
Closes: #18065
sign-batchcmd. With the old logic, when signing multiple transactions the output file is not even a valid json file (checkinvalid_batch.jsonin diff files).Txstype to represent broadcast transactions in a file. Checknew_batch.jsonbroadcast txnow can broadcast both single tx format & multi txs format.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...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers 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...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests