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

chore: Remove Background Voting Loop #59

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

amessbee
Copy link
Contributor

@amessbee amessbee commented Sep 3, 2024

Duplicating similar work in dapp-offer-up of removing voting loop running in the background.

@amessbee amessbee marked this pull request as ready for review September 3, 2024 16:28
@amessbee amessbee requested review from dckc and toliaqat September 3, 2024 16:29
@amessbee amessbee changed the title chore: Remove Background Voting Loop --WIP-- chore: Remove Background Voting Loop Sep 3, 2024
@amessbee amessbee requested a review from mujahidkay September 3, 2024 16:29
echo "done"
}

waitForBlock() (
Copy link
Member

Choose a reason for hiding this comment

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

looks like waitForBlock is still in use. I see a call to it below.

Copy link
Member

Choose a reason for hiding this comment

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

strange that ci passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. In dapp-offer-up, we are pulling the definition of waitForBlock from /usr/src/upgrade-test-scripts/env_setup.sh rather than defining it here. I would rather keep run-chain.sh identical in both dapps. Let me know which one is preferable.

Copy link
Member

Choose a reason for hiding this comment

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

yes, let's keep it identical.

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

Looks like waitForBlock is still used.

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

ok except any removal of the final newline


make -C /workspace/contract mint100

# bring back chain process to foreground
wait
wait
Copy link
Member

Choose a reason for hiding this comment

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

Is that removing the final newline? Let's not do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added back.

wait
Copy link
Member

Choose a reason for hiding this comment

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

This is a fixup (a change to some earlier change in this PR), not a fix (a change in the behavior of the code w.r.t. what was previously released or merged). In agoric-sdk, we auto-squash on merge, but I don't think that's set up here, so please squash and force-push.

Not critical, but let's work toward Tidy Commit History in general.

@amessbee amessbee force-pushed the ms/remove-background-voting-loop branch 3 times, most recently from 0500327 to 9a1bf00 Compare September 10, 2024 07:06
@amessbee amessbee force-pushed the ms/remove-background-voting-loop branch from 9a1bf00 to 2639959 Compare September 10, 2024 07:13
@amessbee amessbee self-assigned this Sep 11, 2024
@amessbee amessbee merged commit 43e9517 into main Sep 11, 2024
2 checks passed
@amessbee amessbee deleted the ms/remove-background-voting-loop branch September 11, 2024 09:40
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.

2 participants