8000 PBTS: Address latest comments on #2231 · Issue #2326 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

PBTS: Address latest comments on #2231 #2326

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.

Sign up for GitHub

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

Jump to bottom
Closed
Tracked by #2063
lasarojc opened this issue Feb 13, 2024 · 6 comments
Closed
Tracked by #2063

PBTS: Address latest comments on #2231 #2326

lasarojc opened this issue Feb 13, 2024 · 6 comments
Labels
abci Application blockchain interface pbts
Milestone
2024-Q1

Comments

@lasarojc
Copy link
Contributor

#2231 was merged to enabled depending PRs to be further worked, but there are some improvements raised by later reviews that should be addressed, probably the most important being the need for extending the UTs.
@lasarojc lasarojc added bug Something isn't working abci Application blockchain interface backport-to-v1.x Tell Mergify to backport the PR to v1.x labels Feb 13, 2024
@github-project-automation github-project-automation bot moved this to Todo in CometBFT Feb 13, 2024
@lasarojc lasarojc removed the bug Something isn't working label Feb 13, 2024
@lasarojc
Copy link
Contributor Author

relates to #2324

@cason cason changed the title Address latest comments on #2231 PBTS: Address latest comments on #2231 Feb 15, 2024
@cason cason added pbts and removed backport-to-v1.x Tell Mergify to backport the PR to v1.x labels Feb 15, 2024
This was referenced Feb 15, 2024
Closed
Merged
@mzabaluev
Copy link
Contributor

I'm wondering about nullable wrapped integer fields like the new vote_extensions_enable_height. Can we just treat 0 as the unset state of the field and keep it a simple int64, since the height 0 is invalid in CometBFT?
Also commented here: #2335 (comment)

@sergio-mena
Copy link
Contributor
sergio-mena commented Feb 20, 2024 •
edited
Loading

@mzabaluev that is not possible, as 0 means "disabled", on both "enable height" fields.

@cason @lasarojc an alternative, which I am thinking of (I'm angry it didn't occur to me earlier), would be adding booleans in the struct rather than wrapping the values. The boolean set to false, meaning "disregard"
8000

@cason
Copy link
Contributor
cason commented Feb 20, 2024

Adding booleans in the struct rather than wrapping the values. The boolean set to false, meaning "disregard"

But to set a value, you have to configure the new height and set the corresponding boolean to true?

@sergio-mena
Copy link
Contributor

@cason yes that's the idea. Similar to the bitarray that @mzabaluev proposed in another comment

@lasarojc
Copy link
Contributor Author

The only pending comment, added here by @mzabaluev , will be handled in the context of #2063 (it was added as a task there)

@github-project-automation github-project-automation bot moved this from Todo to Done in CometBFT Feb 21, 2024
@adizere adizere added this to the 2024-Q1 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
abci Application blockchain interface pbts
Projects
No open projects
CometBFT
Status: Done
Milestone
2024-Q1
Development

No branches or pull requests
5 participants
@cason @mzabaluev @adizere @lasarojc @sergio-mena
0