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

[Feature]: make timeout_height time based #20658

Closed
tac0turtle opened this issue Jun 13, 2024 · 6 comments
Closed

[Feature]: make timeout_height time based #20658

tac0turtle opened this issue Jun 13, 2024 · 6 comments
Assignees

Comments

@tac0turtle
Copy link
Member

Summary

Dydx recently brought up wanting to use time instead of block height for unordered txs. This would need a new field in the TX, which is the opposite direction we wanted to go with making the tx simpler.

Using time is simpler because its easier to understand time as a user than block height since blocks have different times.

Problem Definition

No response

Proposed Feature

Modify timeout_height to be time based instead of block height.

We would need to use the blocks time to check the timeout instead of local time. This would simplify the UX for end users since its easier to understand time over heights.

@hieuvubk
Copy link
Contributor

So user will pass timeout_height as a timestamp right?

@tac0turtle
Copy link
Member Author

we should think about possibly renaming it to timeout. need to have a discussion internally about this

@hieuvubk
Copy link
Contributor

Yep, will be a big refactor

@alpe
Copy link
Contributor

alpe commented Jun 24, 2024

👍 much better UX from a client perspective

@kocubinski
Copy link
Member

Tangibly at the API layer, adding a field like

  google.protobuf.Timestamp timeout_timestamp = 5;

would be non-breaking, and timeout_height (at pos 3) would remain in the spec. So I think it looks like we support both? I tried to estimate the complexity of this but couldn't find where TimeoutHeight is used in the SDK.

@tac0turtle
Copy link
Member Author

tac0turtle commented Jun 24, 2024

Tangibly at the API layer, adding a field like

  google.protobuf.Timestamp timeout_timestamp = 5;

would be non-breaking, and timeout_height (at pos 3) would remain in the spec. So I think it looks like we support both? I tried to estimate the complexity of this but couldn't find where TimeoutHeight is used in the SDK.

we use it here only:

// ValidateTx implements an TxValidator for the TxHeightTimeoutDecorator
// type where the current block height is checked against the tx's height timeout.
// If a height timeout is provided (non-zero) and is less than the current block
// height, then an error is returned.
func (txh TxTimeoutHeightDecorator) ValidateTx(ctx context.Context, tx sdk.Tx) error {
timeoutTx, ok := tx.(TxWithTimeoutHeight)
if !ok {
return errorsmod.Wrap(sdkerrors.ErrTxDecode, "expected tx to implement TxWithTimeoutHeight")
}
timeoutHeight := timeoutTx.GetTimeoutHeight()
headerInfo := txh.env.HeaderService.HeaderInfo(ctx)
if timeoutHeight > 0 && uint64(headerInfo.Height) > timeoutHeight {
return errorsmod.Wrapf(
sdkerrors.ErrTxTimeoutHeight, "block height: %d, timeout height: %d", headerInfo.Height, timeoutHeight,
)
}
return nil
}
, we would make this work with time as well.

@tac0turtle tac0turtle assigned tac0turtle and unassigned tac0turtle Jun 24, 2024
@sontrinh16 sontrinh16 self-assigned this Jul 1, 2024
@tac0turtle tac0turtle moved this from 📋 Backlog to 🤸‍♂️ In Progress in Cosmos-SDK Jul 1, 2024
@tac0turtle tac0turtle moved this from 🤸‍♂️ In Progress to 👀 Waiting / In review in Cosmos-SDK Jul 29, 2024
@github-project-automation github-project-automation bot moved this from 👀 Waiting / In review to 🥳 Done in Cosmos-SDK Jul 29, 2024
@tac0turtle tac0turtle removed this from Cosmos-SDK Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants