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

fix: return 0 height inside GetLastBlockHeight if there are no blocks #76

Merged
merged 3 commits into from
Sep 19, 2022

Conversation

huichiaotsou
Copy link
Contributor

@huichiaotsou huichiaotsou commented Sep 15, 2022

Description

When we use the parse blocks command, in case that the database is empty, return err here will error out the program and not continuing parsing the desired block height. Replacing it here so it's more handy for dev purpose when we need to just test specific heights. (we don't have to run start cmd before using parse blocks all)

Checklist

  • Targeted PR against correct branch.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit tests.
  • Re-reviewed Files changed in the Github PR explorer.

@huichiaotsou huichiaotsou added the automerge Automatically merge PR once all prerequisites pass label Sep 15, 2022
Copy link
Contributor

@MonikaCat MonikaCat 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
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

I'm not sure about this change honestly. Wouldn't it make more sense to have the GetLastBlockHeight method return maybe 0 instead of an error if there are no blocks? I mean, if there are no blocks stored inside the database, 0 makes sense as the last block height (since blocks start from 1 and 0 means no blocks).

@huichiaotsou
Copy link
Contributor Author

I'm not sure about this change honestly. Wouldn't it make more sense to have the GetLastBlockHeight method return maybe 0 instead of an error if there are no blocks? I mean, if there are no blocks stored inside the database, 0 makes sense as the last block height (since blocks start from 1 and 0 means no blocks).

cool I changed the GetLastBlockHeight() method to return 0 when no block is saved. So the parse blocks cmd won't error out as well if there's no block saved.

@huichiaotsou huichiaotsou changed the title fix: replace return err with log.Error() for GetLastBlockHeight() in parse blocks cmd fix: Return 0 as height for GetLastBlockHeight() method while no block is saved Sep 19, 2022
@RiccardoM RiccardoM changed the title fix: Return 0 as height for GetLastBlockHeight() method while no block is saved fix: return 0 height inside GetLastBlockHeight() if there are no blocks Sep 19, 2022
@RiccardoM RiccardoM changed the title fix: return 0 height inside GetLastBlockHeight() if there are no blocks fix: return 0 height inside GetLastBlockHeight if there are no blocks Sep 19, 2022
@mergify mergify bot merged commit ac4f782 into cosmos/v0.44.x Sep 19, 2022
@mergify mergify bot deleted the aaron/parsecmd_last_block_db_height branch September 19, 2022 07:55
mergify bot pushed a commit that referenced this pull request Sep 19, 2022
…e no blocks" (#78)

## Description

fix #76 
I forgot to remove this section of codes lol

## Checklist
- [ ] Targeted PR against correct branch.
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Wrote unit tests.  
- [ ] Re-reviewed `Files changed` in the Github PR explorer.
RiccardoM pushed a commit that referenced this pull request Sep 20, 2022
…#76)

When we use the parse blocks command, in case that the database is empty, `return err` here will error out the program and not continuing parsing the desired block height. Replacing it here so it's more handy for dev purpose when we need to just test specific heights. (we don't have to run ` start cmd` before using `parse blocks all`)

- [x] Targeted PR against correct branch.
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Wrote unit tests.
- [x] Re-reviewed `Files changed` in the Github PR explorer.
RiccardoM pushed a commit that referenced this pull request Sep 20, 2022
…e no blocks" (#78)

## Description

fix #76 
I forgot to remove this section of codes lol

## Checklist
- [ ] Targeted PR against correct branch.
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Wrote unit tests.  
- [ ] Re-reviewed `Files changed` in the Github PR explorer.
MonikaCat pushed a commit that referenced this pull request Jan 19, 2024
…#76)

When we use the parse blocks command, in case that the database is empty, `return err` here will error out the program and not continuing parsing the desired block height. Replacing it here so it's more handy for dev purpose when we need to just test specific heights. (we don't have to run ` start cmd` before using `parse blocks all`)

- [x] Targeted PR against correct branch.
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Wrote unit tests.
- [x] Re-reviewed `Files changed` in the Github PR explorer.
MonikaCat pushed a commit that referenced this pull request Jan 19, 2024
…e no blocks" (#78)

## Description

fix #76 
I forgot to remove this section of codes lol

## Checklist
- [ ] Targeted PR against correct branch.
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Wrote unit tests.  
- [ ] Re-reviewed `Files changed` in the Github PR explorer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge PR once all prerequisites pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants