refactor: signer to use txstatus #3767
Conversation
ninabarbakadze
force-pushed
the
nina/signer-txstatus
branch
from
6302ec5
to
8596e69
Compare
ninabarbakadze
requested review from
rach-id,
evan-forbes and
a team
as code owners
WalkthroughWalkthroughThe changes primarily enhance transaction handling and testing by incorporating the Changes
Assessment against linked 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 Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (5)
app/test/big_blob_test.go (1)
86-87: Reminder: Address the FIXME comment.The comment indicates an intention to assert
RawLogonceTxStatussupports it. Ensure to track this update.Consider opening a tracking issue or adding a more detailed comment for future reference.
x/blobstream/integration_test.go (1)
93-99: Reminder: Address the FIXME comment.The comment indicates a temporary solution for querying the raw log. Ensure to update this once
TxStatussupports the feature.Consider opening a tracking issue or adding a more detailed comment for future reference.
app/test/prepare_proposal_context_test.go (1)
84-90: Reminder: Address the FIXME comment.The comment indicates a temporary solution for querying the raw log. Ensure to update this once
TxStatussupports the feature.Consider opening a tracking issue or adding a more detailed comment for future reference.
pkg/user/tx_client_test.go (1)
50-52: Track the resolution of the temporary solution.The initialization of
serviceClientis marked as a temporary solution with a FIXME comment. Ensure this is tracked for future resolution.- // FIXME: Temporary way of querying the raw log. + // FIXME: Temporary way of querying the raw log. Track this issue for future resolution.app/test/std_sdk_test.go (1)
329-335: Track the resolution of the temporary solution.The use of
serviceClient.GetTximproves transaction result validation. The FIXME comment indicates a temporary solution that should be tracked for future resolution.- // FIXME: Temporary way of querying the raw log. + // FIXME: Temporary way of querying the raw log. Track this issue for future resolution.
| // FIXME: Temporary way of querying the raw log. | ||
| // TxStatus will natively support this in the future. | ||
| suite.serviceClient = sdktx.NewServiceClient(suite.ctx.GRPCClient) |
There was a problem hiding this comment.
Well we will still need this to get the gas wanted and gas used
There was a problem hiding this comment.
true i think i automatically copied this. will remove!
| if err != nil && resp != nil { | ||
| return &TxResponse{Code: resp.Code, TxHash: resp.TxHash}, fmt.Errorf("failed to broadcast tx: %v", err) | ||
| } else if err != nil { | ||
| return &TxResponse{}, fmt.Errorf("failed to broadcast tx: %v", err) | ||
| } |
There was a problem hiding this comment.
nit: would be a little cleaner to use a helper function:
if err != nil {
return parseTxResp(resp), fmt.Errorf("failed to broadcast tx: %w", err)
}
which turns the broadcastTxResponse to a TxResponse or nil if it's nil
There was a problem hiding this comment.
yeah i considered it but I thought that maybe this was more straight-forward and easy to understand
| Code: resp.ExecutionCode, | ||
| } | ||
| if resp.ExecutionCode != 0 { | ||
| return txResponse, fmt.Errorf("tx was included but failed with code %d: %s", resp.ExecutionCode, resp.Status) |
There was a problem hiding this comment.
[not blocking]
using the word committed instead of included here I think is a smidge clearer as there's a less of a chance to confuse with inclusion into the mempool
| return txResponse, fmt.Errorf("tx was included but failed with code %d: %s", resp.ExecutionCode, resp.Status) | |
| return txResponse, fmt.Errorf("tx was committed but failed with code %d: %s", resp.ExecutionCode, resp.Status) |
There was a problem hiding this comment.
I'll address this in a follow-up
| // TxResponse is a response from the chain after | ||
| // a transaction has been submitted. | ||
| type TxResponse struct { | ||
| Height int64 | ||
| TxHash string | ||
| Code uint32 | ||
| } |
There was a problem hiding this comment.
what's the benefit of having our own type here? if this is different from the sdk TxResponse, is there a way to indicate it in the name?
There was a problem hiding this comment.
because for example in SubmitTx BroadcastTx and ConfirmTx have different responses now that we've switched to a new indexer
| case <-ctx.Done(): | ||
| return &sdktypes.TxResponse{}, ctx.Err() | ||
| case <-pollTicker.C: | ||
| if err == nil && resp != nil { |
There was a problem hiding this comment.
where are we notifying the user that their tx is in the mempool?
There was a problem hiding this comment.
I don't think we need to notify a user in this instance cause we're just confirming the inclusion in a block
ninabarbakadze
added
the
backport:v2.x
## Overview Fixes #3366 Opens - celestiaorg/celestia-core#1453 and celestiaorg/celestia-core#1454 (cherry picked from commit a28b9e7) # Conflicts: # pkg/user/tx_client.go
Overview
Fixes #3366
Opens - celestiaorg/celestia-core#1453 and celestiaorg/celestia-core#1454