refactor(auth): refactor auth/tx to use x/tx #19224
Conversation
testinginprod
changed the title
testinginprod
changed the title
WalkthroughWalkthroughThe changes in this set primarily revolve around the migration and restructuring of transaction-related functionality within the codebase. This includes updating module paths, eliminating obsolete dependencies, and extensively refactoring the transaction building process. Moreover, there is a transition from utilizing Changes
Related issues
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 your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
x/auth/tx/query.go
Outdated
| // deprecating (StdTxConfig support) | ||
| type intoAny interface { | ||
| AsAny() *codectypes.Any | ||
| panic("todo") |
There was a problem hiding this comment.
The mkTxResult function has been altered to panic with a "todo" message.
- panic("todo")
+ // TODO: Implement the correct logic for mkTxResult or remove if not needed.This is a placeholder for future implementation and should be replaced with the correct logic or removed if not needed.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| panic("todo") | |
| // TODO: Implement the correct logic for mkTxResult or remove if not needed. |
x/auth/tx/service.go
Outdated
| if req.Tx == nil { | ||
| return nil, status.Error(codes.InvalidArgument, "invalid empty tx") | ||
| } | ||
| panic("todo") |
There was a problem hiding this comment.
The TxEncode function has been modified to panic with a "todo" message.
- panic("todo")
+ // TODO: Implement the correct logic for TxEncode or remove if not needed.This is a placeholder for future implementation and should be replaced with the correct logic or removed if not needed.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| panic("todo") | |
| // TODO: Implement the correct logic for TxEncode or remove if not needed. |
x/auth/tx/service.go
Outdated
| if req.TxBytes == nil { | ||
| return nil, status.Error(codes.InvalidArgument, "invalid empty tx bytes") | ||
| } | ||
| panic("todo") |
There was a problem hiding this comment.
The TxDecode function has been modified to panic with a "todo" message.
- panic("todo")
+ // TODO: Implement the correct logic for TxDecode or remove if not needed.This is a placeholder for future implementation and should be replaced with the correct logic or removed if not needed.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| panic("todo") | |
| // TODO: Implement the correct logic for TxDecode or remove if not needed. |
x/auth/tx/builder.go
Outdated
| func intoV2Fees(fees sdk.Coins) []*basev1beta1.Coin { | ||
| coins := make([]*basev1beta1.Coin, len(fees)) | ||
| for i, c := range fees { | ||
| coins[i] = &basev1beta1.Coin{ | ||
| Denom: c.Denom, | ||
| Amount: c.Amount.String(), | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The intoV2Fees function is intended to convert sdk.Coins to a slice of *basev1beta1.Coin, but it returns nil instead of the converted slice. This is likely a bug.
func intoV2Fees(fees sdk.Coins) []*basev1beta1.Coin {
coins := make([]*basev1beta1.Coin, len(fees))
for i, c := range fees {
coins[i] = &basev1beta1.Coin{
Denom: c.Denom,
Amount: c.Amount.String(),
}
}
- return nil
+ return coins
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func intoV2Fees(fees sdk.Coins) []*basev1beta1.Coin { | |
| coins := make([]*basev1beta1.Coin, len(fees)) | |
| for i, c := range fees { | |
| coins[i] = &basev1beta1.Coin{ | |
| Denom: c.Denom, | |
| Amount: c.Amount.String(), | |
| } | |
| } | |
| return nil | |
| } | |
| func intoV2Fees(fees sdk.Coins) []*basev1beta1.Coin { | |
| coins := make([]*basev1beta1.Coin, len(fees)) | |
| for i, c := range fees { | |
| coins[i] = &basev1beta1.Coin{ | |
| Denom: c.Denom, | |
| Amount: c.Amount.String(), | |
| } | |
| } | |
| return coins | |
| } |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
x/auth/tx/testutil/suite.go
Outdated
| tx := txBuilder.GetTx() | ||
| newTxBldr, err := s.TxConfig.WrapTxBuilder(tx) | ||
| s.Require().NoError(err) | ||
| txBuilder.SetFeePayer(tx.FeePayer()) // NOTE: fee payer will be populated even if empty. |
There was a problem hiding this comment.
The code correctly retrieves the transaction from txBuilder and wraps it using WrapTxBuilder. However, the assertion s.Require().Equal(txBuilder, newTxBldr) is incorrect because txBuilder and newTxBldr are two different instances of TxBuilder. The assertion should compare the relevant fields of both builders to ensure they are equal after wrapping.
- s.Require().Equal(txBuilder, newTxBldr)
+ // Ensure that the relevant fields of txBuilder and newTxBldr are equal
+ s.Require().Equal(txBuilder.GetTx(), newTxBldr.GetTx())Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| tx := txBuilder.GetTx() | |
| newTxBldr, err := s.TxConfig.WrapTxBuilder(tx) | |
| s.Require().NoError(err) | |
| txBuilder.SetFeePayer(tx.FeePayer()) // NOTE: fee payer will be populated even if empty. | |
| tx := txBuilder.GetTx() | |
| newTxBldr, err := s.TxConfig.WrapTxBuilder(tx) | |
| s.Require().NoError(err) | |
| txBuilder.SetFeePayer(tx.FeePayer()) // NOTE: fee payer will be populated even if empty. | |
| // Ensure that the relevant fields of txBuilder and newTxBldr are equal | |
| s.Require().Equal(txBuilder.GetTx(), newTxBldr.GetTx()) |
|
|
LGTM, there are some tests to fix |
| @@ -982,6 +982,7 @@ func (s *CLITestSuite) TestSignWithMultiSignersAminoJSON() { | |||
| } | |||
|
|
|||
| func (s *CLITestSuite) TestAuxSigner() { | |||
| s.T().Skip("re-enable this when we bring back sign mode aux client testing") | |||
There was a problem hiding this comment.
what's rationale for disabling this test?
There was a problem hiding this comment.
It was left out of scope for this PR and was removed from the builder. This needs to be added to client/v2, just created an issue: #19433.
cc: @testinginprod
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yml
Files selected for processing (5)
- baseapp/abci_test.go (1 hunks)
- baseapp/utils_test.go (2 hunks)
- x/auth/go.mod (1 hunks)
- x/auth/go.sum (1 hunks)
- x/auth/tx/builder.go (2 hunks)
Files skipped from review due to trivial changes (1)
- baseapp/abci_test.go
Additional comments: 4
x/auth/go.mod (1)
- 169-169: Ensure the relative path replacements for
cosmossdk.io/apiand othercosmossdk.iomodules are correctly pointing to the intended local directories. This change impacts how modules are resolved and can affect build and dependency resolution.baseapp/utils_test.go (2)
- 355-355: Ensure the hardcoded
Signervalue insetFailOnHandlerfunction is appropriate for the test scenarios it's used in. Hardcoding addresses in tests can be acceptable but verify it aligns with the test's intent.- 371-373: Verify the hardcoded
Signervalue inwonkyMsgfunction is intended for the specific test cases. Using hardcoded values in tests is common, but it's important to ensure they match the testing requirements.x/auth/go.sum (1)
- 1-5: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1000]
Confirmed the removal of
cosmossdk.io/apimodule versionv0.7.3-0.20231113122742-912390d5fc4aand its associatedgo.modentry as mentioned in the summary. No other unexpected changes related to this action were found in thego.sumfile.
| func (w *builder) GetTx() authsign.Tx { | ||
| buildTx, err := w.getTx() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if len(sigs) != len(signers) { | ||
| return errorsmod.Wrapf( | ||
| sdkerrors.ErrUnauthorized, | ||
| "wrong number of signers; expected %d, got %d", len(signers), len(sigs), | ||
| ) | ||
| panic(err) |
There was a problem hiding this comment.
Replace panic with proper error handling in GetTx method to avoid potential runtime crashes.
- panic(err)
+ return nil, errCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (w *builder) GetTx() authsign.Tx { | |
| buildTx, err := w.getTx() | |
| if err != nil { | |
| return err | |
| } | |
| if len(sigs) != len(signers) { | |
| return errorsmod.Wrapf( | |
| sdkerrors.ErrUnauthorized, | |
| "wrong number of signers; expected %d, got %d", len(signers), len(sigs), | |
| ) | |
| panic(err) | |
| func (w *builder) GetTx() authsign.Tx { | |
| buildTx, err := w.getTx() | |
| if err != nil { | |
| return nil, err |
| func (w *wrapper) SetSignatures(signatures ...signing.SignatureV2) error { | ||
| func (w *builder) SetFeeGranter(feeGranter sdk.AccAddress) { w.granter = feeGranter } | ||
|
|
||
| func (w *builder) SetSignatures(signatures ...signing.SignatureV2) error { |
There was a problem hiding this comment.
Review the complexity of SetSignatures method for potential refactoring opportunities to improve readability and maintainability.
| func (w *wrapper) setSignatures(sigs [][]byte) { | ||
| w.tx.Signatures = sigs | ||
| } | ||
| func (w *builder) AddAuxSignerData(data tx.AuxSignerData) error { return fmt.Errorf("not supported") } |
There was a problem hiding this comment.
Consider adding a TODO comment for future implementation or decision regarding the AddAuxSignerData method.
+ // TODO: Implement AddAuxSignerData functionality or remove if not plannedCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (w *builder) AddAuxSignerData(data tx.AuxSignerData) error { return fmt.Errorf("not supported") } | |
| // TODO: Implement AddAuxSignerData functionality or remove if not planned | |
| func (w *builder) AddAuxSignerData(data tx.AuxSignerData) error { return fmt.Errorf("not supported") } |
|
closes #17530 |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/auth/tx/config/depinject.go (3 hunks)
Additional comments: 2
x/auth/tx/config/depinject.go (2)
- 7-7: The import of
github.com/cosmos/gogoproto/protohas been added. This change aligns with the PR's objective to modernize the authentication and transaction module by utilizing new types and methods introduced in v1beta1. Ensure that this new import is utilized effectively in the codebase, particularly in the context of the changes made toProvideProtoRegistry.- 64-64: The return value in
ProvideProtoRegistryhas been changed togogoproto.HybridResolver. This modification is crucial for the refactoring effort, as it indicates a shift towards using a hybrid resolver for proto file resolution, which is likely part of the broader effort to align with v1beta1 standards. It's important to verify that this change integrates well with the rest of the system, especially in terms of how proto files are resolved throughout the application. Given the context of the PR, this change seems appropriate, but it's essential to ensure that all downstream dependencies that rely onProvideProtoRegistryare compatible with this new return value.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yml
Files selected for processing (6)
- x/auth/go.mod (1 hunks)
- x/auth/go.sum (1 hunks)
- x/slashing/go.mod (1 hunks)
- x/slashing/go.sum (1 hunks)
- x/tx/go.mod (1 hunks)
- x/tx/go.sum (1 hunks)
Files skipped from review due to trivial changes (1)
- x/tx/go.sum
Files skipped from review as they are similar to previous changes (1)
- x/auth/go.sum
Additional comments: 1
x/slashing/go.sum (1)
- 1-1: The removal of the
cosmossdk.io/apimodule versionv0.7.3-0.20231113122742-912390d5fc4afrom thego.sumfile indicates that this specific version is no longer required by the project. It's important to ensure that this change is intentional and that all necessary dependencies are correctly specified in thego.modfile. Runninggo mod tidycan help verify that the project's dependencies are properly managed and consistent.
| @@ -34,3 +34,5 @@ require ( | |||
| google.golang.org/grpc v1.61.1 // indirect | |||
| gopkg.in/yaml.v3 v3.0.1 // indirect | |||
| ) | |||
|
|
|||
| replace cosmossdk.io/api => ../../api | |||
There was a problem hiding this comment.
The addition of the replace directive for cosmossdk.io/api to point to ../../api is a significant change. This directive overrides the module's source location, which can be particularly useful for local development or when working with forked versions of a module. However, it's crucial to ensure that this change aligns with the project's dependency management strategy and that the relative path ../../api correctly points to the intended module location within the project's structure. If this change is meant to be temporary or only for development purposes, consider documenting this context either within the go.mod file itself or in relevant project documentation to avoid confusion.
| @@ -167,6 +167,7 @@ replace github.com/cosmos/cosmos-sdk => ../../. | |||
|
|
|||
| // TODO remove post spinning out all modules | |||
| replace ( | |||
| cosmossdk.io/api => ../../api | |||
There was a problem hiding this comment.
The replace directives in this go.mod file significantly alter the source locations for various cosmossdk.io modules, pointing them to local paths within the project. This approach is beneficial for local development or when making coordinated changes across multiple modules. However, it's essential to verify that these local paths (../../api, ../../core, ../../depinject, ../accounts, ../bank, ../staking, ../tx) accurately reflect the project's directory structure and that these changes are aligned with the project's overall dependency management strategy. If these directives are intended for temporary or development-specific purposes, documenting their intended use can help maintain clarity and prevent potential issues in dependency resolution for other developers or in production environments.
| @@ -168,6 +168,7 @@ replace github.com/cosmos/cosmos-sdk => ../../. | |||
|
|
|||
| // TODO remove post spinning out all modules | |||
| replace ( | |||
| cosmossdk.io/api => ../../api | |||
There was a problem hiding this comment.
The replace directives in the x/slashing module's go.mod file, similar to those in the x/tx and x/auth modules, redirect the source locations for various cosmossdk.io modules to local paths. This pattern indicates a structured approach to managing dependencies across these modules, likely to facilitate local development or coordinated updates. As with the other modules, it's critical to ensure that these local paths (../../api, ../../core, ../../depinject, ../accounts, ../auth, ../bank, ../staking, ../tx) are accurate and reflect the intended structure of the project. Additionally, documenting the purpose and scope of these directives can help maintain clarity and prevent confusion during development or when integrating changes into broader environments.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- x/tx/go.mod (2 hunks)
- x/tx/go.sum (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/tx/go.mod
Additional comments: 2
x/tx/go.sum (2)
- 1-4: The addition of new dependencies from
buf.buildforcosmos/gogo-protoandtendermint/tendermintprotocol buffers is noted. Ensure these versions are compatible with the project's requirements and check for any potential security vulnerabilities in these versions.- 1-4: Confirm the removal of
cosmossdk.io/apimodule versionv0.7.2was intentional and assess if it impacts other parts of the project. This change should be aligned with the PR's objectives and not inadvertently affect functionality.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (5)
- tests/go.mod (1 hunks)
- x/auth/go.mod (2 hunks)
- x/auth/go.sum (1 hunks)
- x/slashing/go.mod (2 hunks)
- x/slashing/go.sum (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/auth/go.sum
- x/slashing/go.mod
- x/slashing/go.sum
Additional comments: 2
x/auth/go.mod (1)
- 172-172: The
replacedirectives in thisgo.modfile significantly alter the source locations for variouscosmossdk.iomodules, pointing them to local paths within the project. This approach is beneficial for local development or when making coordinated changes across multiple modules. However, it's essential to verify that these local paths (../../api,../../core,../../depinject,../accounts,../bank,../staking,../tx) accurately reflect the project's directory structure and that these changes are aligned with the project's overall dependency management strategy. If these directives are intended for temporary or development-specific purposes, documenting their intended use can help maintain clarity and prevent potential issues in dependency resolution for other developers or in production environments.tests/go.mod (1)
- 10-10: The addition of
cosmossdk.io/errors v1.0.1 // indirectin thetests/go.modfile indicates an update in the dependencies that indirectly affects this module. It's good practice to keep dependencies up-to-date for security and functionality improvements. However, ensure that this update does not introduce breaking changes or incompatibilities with the current test suite. It might be beneficial to verify through additional testing or review of theerrorsmodule's changelog for significant changes.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (2)
- baseapp/abci_utils_test.go (5 hunks)
- x/tx/signing/textual/e2e_test.go (1 hunks)
Additional comments: 4
baseapp/abci_utils_test.go (4)
- 472-480: The updated transaction size assertions reflect changes in the transaction encoding or structure. Ensure these new sizes are accurately calculated based on the current transaction encoding scheme to prevent flaky tests.
Consider adding a comment or documentation on how these sizes were determined for future reference and maintenance.
- 493-493: The
MaxTxBytesvalue in the test case "skip same-sender non-sequential sequence and then add others txs" is set to the sum of the sizes of the first and fourth transactions. Ensure this value aligns with the intended test scenario, especially considering the transaction overhead and the mempool's behavior.Verification successful
The output from the executed scripts provides insight into the test case "skip same-sender non-sequential sequence and then add others txs" and various settings of
MaxTxBytesin different test scenarios withinbaseapp/abci_utils_test.go. Specifically, for the test case in question,MaxTxBytesis set to the sum of 180 + 181, which matches the description in the review comment. Other test cases show different values forMaxTxBytes, indicating that this parameter is adjusted based on the specific requirements of each test scenario.However, the output does not explicitly detail the rationale behind the specific sum of 180 + 181 for
MaxTxBytes, nor does it provide information on transaction sizes or overhead considerations directly. The values seem to be tailored to the test scenarios, possibly to test the mempool's behavior under different conditions, but without further context or explanation in the output, the intention behind the exact value ofMaxTxBytesin the highlighted test case remains inferred from the setup.Given the information provided, the review comment's concern about ensuring the
MaxTxBytesvalue aligns with the intended test scenario, especially considering transaction overhead and the mempool's behavior, is valid. However, without explicit details on the calculation or considerations behind the specific value, the verification process cannot conclusively determine the correctness of theMaxTxBytessetting beyond its intentional use in the test setup.Based on the available context, the review comment is upheld as it correctly identifies a point of interest regarding the
MaxTxBytessetting in the test scenario. The verification process confirms the setting as described but cannot fully ascertain the rationale without further context.* 501-501: Similar to the previous comment, verify that the `MaxTxBytes` value in the test case "skip multi-signers msg non-sequential sequence" accurately reflects the intended test conditions, taking into account transaction sizes and mempool selection logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the calculation of MaxTxBytes in relation to transaction overhead and mempool behavior.Length of output: 0
Script:
#!/bin/bash # Inspect the test case "skip same-sender non-sequential sequence and then add others txs" for details on transaction sizes and overhead considerations grep -C 5 "skip same-sender non-sequential sequence and then add others txs" baseapp/abci_utils_test.go # Look for any documentation or comments related to MaxTxBytes calculation in the test file grep -i "MaxTxBytes" baseapp/abci_utils_test.go -A 3 -B 3Length of output: 2099
Verification successful
Given the lack of output from the previous script and the need to verify the
MaxTxBytesvalue in multi-signer scenarios, let's attempt a more targeted approach to gather the necessary information. We'll search the codebase for references toMaxTxBytesin the context of multi-signer transactions, including documentation, comments, or other test cases that might shed light on howMaxTxBytesshould be calculated or adjusted for such scenarios.
The search results indicate that
MaxTxBytesis consistently used across various test cases and is an important factor in transaction selection logic, including for multi-signer scenarios. The specific value ofMaxTxBytesin the test case "skip multi-signers msg non-sequential sequence" (263 + 264) is likely chosen based on the sizes of the transactions involved and the intended test conditions, reflecting a calculated decision to test the mempool's behavior under specific conditions. Without direct evidence to question the justification of this value, it can be concluded that theMaxTxBytesvalue accurately reflects the intended test conditions, taking into account transaction sizes and mempool selection logic.* 510-510: For the test case "only the first tx is added," ensure that the `MaxTxBytes` value is set with a clear understanding of how it influences the selection of transactions in the mempool, particularly in scenarios with sequential and non-sequential transactions.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Further investigation into the calculation of MaxTxBytes for multi-signer scenarios.Length of output: 0
Script:
#!/bin/bash # Search for references to MaxTxBytes in the context of multi-signer transactions. # Look for comments or documentation related to MaxTxBytes and multi-signer transactions. rg "MaxTxBytes" -C 5 # Search for test cases involving multi-signer transactions that might reference MaxTxBytes. rg "multi-signer" -C 5Length of output: 18825
Verification successful
Given the lack of output from the initial script and the need to understand how
MaxTxBytesinfluences transaction selection in sequential and non-sequential scenarios, we will generate new shell scripts to search for relevant documentation, comments, and code. This will help us verify the impact ofMaxTxByteson transaction selection.
The output from the shell scripts provides insight into how
MaxTxBytesinfluences transaction selection in the context of the test cases withinbaseapp/abci_utils_test.go. The test cases demonstrate various scenarios whereMaxTxBytesis set to different values, affecting the number of transactions that can be included in a proposal. For example, settingMaxTxBytesto a small value results in fewer or no transactions being added, while larger values allow more transactions to be included. This directly correlates with the original review comment's concern about understanding the impact ofMaxTxByteson transaction selection, especially in scenarios with sequential and non-sequential transactions.The test case "only the first tx is added" specifically sets
MaxTxBytesto263 + 264, indicating a calculated approach to determining the maximum size of transactions that can be included. This, along with the other test cases, shows a clear understanding of howMaxTxBytesinfluences the selection of transactions in the mempool, aligning with the intent of the review comment to ensure thatMaxTxBytesis set with a clear understanding of its impact.Given the detailed test cases and the explicit handling of
MaxTxBytesin both sequential and non-sequential transaction scenarios, it can be concluded that the code demonstrates a clear understanding of howMaxTxBytesinfluences transaction selection in the mempool.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Examine the impact of MaxTxBytes on transaction selection in sequential and non-sequential scenarios.Length of output: 0
Script:
#!/bin/bash # Search for documentation or comments related to MaxTxBytes and its impact on transaction selection. rg "MaxTxBytes" -C 3 # Examine the test cases in baseapp/abci_utils_test.go for usage of MaxTxBytes rg "MaxTxBytes" baseapp/abci_utils_test.go -C 10 # Look into the mempool implementation or related logic that handles MaxTxBytes rg "mempool" -C 3 | grep "MaxTxBytes" -C 3Length of output: 14112
| decodeWant, err := hex.DecodeString(tc.Cbor) | ||
| require.NoError(t, err) | ||
| t.Log("got: " + string(signDoc)) | ||
| t.Log("want " + string(decodeWant)) |
There was a problem hiding this comment.
The addition of decoding tc.Cbor using hex.DecodeString, logging the signDoc and decodeWant, and comparing tc.Cbor with hex.EncodeToString(signDoc) enhances the test's clarity and debuggability. However, consider adding error handling for the hex.EncodeToString(signDoc) call to ensure robustness.
+ encodedSignDoc := hex.EncodeToString(signDoc)
+ require.NoError(t, err)
+ require.Equal(t, tc.Cbor, encodedSignDoc)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| decodeWant, err := hex.DecodeString(tc.Cbor) | |
| require.NoError(t, err) | |
| t.Log("got: " + string(signDoc)) | |
| t.Log("want " + string(decodeWant)) | |
| decodeWant, err := hex.DecodeString(tc.Cbor) | |
| require.NoError(t, err) | |
| t.Log("got: " + string(signDoc)) | |
| t.Log("want " + string(decodeWant)) | |
| encodedSignDoc := hex.EncodeToString(signDoc) | |
| require.NoError(t, err) | |
| require.Equal(t, tc.Cbor, encodedSignDoc) |
Description
Closes: #XXXX
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
Summary by CodeRabbit