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

Deprecate status get height #1634

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

fluidvanadium
Copy link
Contributor

This function used to have a warning comment. Now it has a deprecation. It recently caused a bug for a member of the team that other people have used this function.

This PR cant merge as is because clippy wont like it.

@zancas
Copy link
Member

zancas commented Jan 29, 2025

Why can't this merge?

@Oscar-Pepper
Copy link
Contributor

get_height has very useful utility, I don't think we should remove functions that serve a unique purpose because people are not to careful enough to misuse them. I will take a look at how I have used in sync

@fluidvanadium
Copy link
Contributor Author

get height returns a value that either represents when the transaction was Calculated, Transmitted, Mempool, or Confirmed. These numbers are typically different by a couple of blocks.
then it forgets which case

it has been useful as a blunt instrument, for example in reorgs. but it has caused Problems

@Oscar-Pepper
Copy link
Contributor

get height returns a value that either represents when the transaction was Calculated, Transmitted, Mempool, or Confirmed. These numbers are typically different by a couple of blocks. then it forgets which case

it has been useful as a blunt instrument, for example in reorgs. but it has caused Problems

ok could we replace it with a "get_pending_height" then so we can access the height of unconfirmed txs also then?

@Oscar-Pepper
Copy link
Contributor

well, there is a couple of times that I will need to write uneccessary logic to retrieve the height if this method didnt exist. I think its best we add the "pending height" method and just push to use confirmed_height and pending_height where possible and only get_height if its necessary

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.

3 participants