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.

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

Node/Gov: Add target to db #3791

Merged
merged 3 commits into from
Mar 5, 2024
Merged

Node/Gov: Add target to db #3791

merged 3 commits into from
Mar 5, 2024

Conversation

bruce-riley
Copy link
Contributor

This PR adds the target chain and address to the governor transfers stored in the DB. It also handles reloading transfers that do not have those fields and migrating them to the new format.

@bruce-riley bruce-riley force-pushed the node/gov_add_target_to_db branch 3 times, most recently from 8ca12a5 to 62e5f98 Compare February 17, 2024 15:04
@bruce-riley bruce-riley marked this pull request as ready for review February 19, 2024 00:40
SEJeff
SEJeff previously approved these changes Feb 20, 2024
View reviewed changes
Copy link
Collaborator

@SEJeff SEJeff left a comment

Choose a reason for hiding this comment

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

My only concern here is that there are now new error conditions in the governor code that don't seem to have explicit tests for them. I do not love that, but the change seems very straightforward and sensible.

@johnsaigle johnsaigle mentioned this pull request Feb 20, 2024
Merged
djb15
djb15 previously approved these changes Feb 20, 2024
View reviewed changes
@bruce-riley bruce-riley dismissed stale reviews from djb15 and SEJeff via c605aad February 22, 2024 13:50
@bruce-riley bruce-riley force-pushed the node/gov_add_target_to_db branch from c605aad to b45d86f Compare February 22, 2024 14:16
panoel
panoel previously approved these changes Feb 22, 2024
View reviewed changes
@bruce-riley bruce-riley force-pushed the node/gov_add_target_to_db branch from b45d86f to 5022018 Compare March 4, 2024 17:08
@bruce-riley bruce-riley force-pushed the node/gov_add_target_to_db branch 4 times, most recently from bb7571d to df47f9c Compare March 4, 2024 22:45
@KriptoLatino
Copy link

node/pkg/db/governor.go

@bruce-riley bruce-riley force-pushed the node/gov_add_target_to_db branch from df47f9c to 4a63c48 Compare March 5, 2024 13:56
@bruce-riley bruce-riley force-pushed the node/gov_add_target_to_db branch from 4a63c48 to 99dc44d Compare March 5, 2024 13:57
@bruce-riley
Copy link
Contributor Author

@SEJeff I've added more tests.

SEJeff
SEJeff previously approved these changes Mar 5, 2024
View reviewed changes
Copy link
Collaborator

@SEJeff SEJeff left a comment

Choose a reason for hiding this comment

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

I didn't re-review the entire thing, only the updated commit. This is great.

@bruce-riley bruce-riley requested a review from SEJeff March 5, 2024 14:56
@bruce-riley bruce-riley requested a review from panoel March 5, 2024 15:24
@bruce-riley bruce-riley merged commit 6746683 into main Mar 5, 2024
22 checks passed
@bruce-riley bruce-riley deleted the node/gov_add_target_to_db branch March 5, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SEJeff SEJeff SEJeff approved these changes

@panoel panoel panoel approved these changes

@djb15 djb15 djb15 left review comments

@johnsaigle johnsaigle Awaiting requested review from johnsaigle johnsaigle is a code owner

@claudijd claudijd Awaiting requested review from claudijd

@kev1n-peters kev1n-peters Awaiting requested review from kev1n-peters

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bruce-riley @KriptoLatino @SEJeff @djb15 @panoel