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

chore: backport tx status #1405

Conversation

ninabarbakadze
Copy link
Member

@ninabarbakadze ninabarbakadze commented Jun 28, 2024

Description

backport TxStatus work from main

@ninabarbakadze ninabarbakadze changed the title Nina/backport tx status chore: backport tx status Jun 28, 2024
@ninabarbakadze ninabarbakadze marked this pull request as ready for review June 30, 2024 09:59
@ninabarbakadze ninabarbakadze requested a review from a team as a code owner June 30, 2024 09:59
@ninabarbakadze ninabarbakadze requested review from rootulp, rach-id and cmwaters and removed request for a team and rach-id June 30, 2024 09:59
cmwaters
cmwaters previously approved these changes Jul 1, 2024
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first pass looks good to me 👍
Is there an issue for this or more context?

rpc/client/http/http.go Outdated Show resolved Hide resolved
Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>
@ninabarbakadze
Copy link
Member Author

first pass looks good to me 👍 Is there an issue for this or more context?

Thanks for the review! Here are the links to the work I'm backporting from main:

Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAck.

Awesome stuff 🔥 Left some unblocking nits.

Also, would be good to update the openapi specs to reflect these changes both in main in this branch. Can be handled in a separate PR or an issue for this to be done later.

@@ -17,6 +17,13 @@ import (
"github.com/tendermint/tendermint/types"
)

const (
txStatusUnknown string = "UNKNOWN"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[non-blocking][microscopic nit]
probably document the cases where the status is unknown

Comment on lines +66 to +69
Height int64 `json:"height"`
Index uint32 `json:"index"`
ExecutionCode uint32 `json:"execution_code"`
Status string `json:"status"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[non blocking]
maybe document these

@ninabarbakadze
Copy link
Member Author

utAck.

Awesome stuff 🔥 Left some unblocking nits.

Also, would be good to update the openapi specs to reflect these changes both in main in this branch. Can be handled in a separate PR or an issue for this to be done later.

Opened an issue #1406

@ninabarbakadze ninabarbakadze merged commit b1d52fd into celestiaorg:v0.34.x-celestia Jul 2, 2024
18 of 19 checks passed
@ninabarbakadze ninabarbakadze deleted the nina/backport-tx-status branch July 2, 2024 17:11
rootulp pushed a commit to rootulp/celestia-core that referenced this pull request Sep 20, 2024
…tiaorg#1405) (celestiaorg#1407)

* doc: improve documentation of BlockParams.MaxBytes (manual backport of celestiaorg#1405)

* Applying @sergio-mena suggestion

Co-authored-by: Sergio Mena <sergio@informal.systems>

* Applying @jmalicevic suggestion

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>

---------

Co-authored-by: Sergio Mena <sergio@informal.systems>
Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants