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

[DPA-1067]: feat: support enable disable feeds managers/JD #15010

Merged
merged 3 commits into from
Oct 30, 2024

Conversation

ChrisAmora
Copy link
Contributor

@ChrisAmora ChrisAmora commented Oct 29, 2024

  • Add Enable / Disable Feeds Manager to UI (bringing the latest changes from this operator ui PR)
  • Add disableAt check at connection even without multi managers flag.

JIRA: https://smartcontract-it.atlassian.net/browse/DPA-1067

@graham-chainlink graham-chainlink changed the title Feat/enable disable jd mutations UI feat: support enable disable feeds managers/JD Oct 30, 2024
@graham-chainlink graham-chainlink marked this pull request as ready for review October 30, 2024 01:55
@graham-chainlink graham-chainlink requested review from eutopian, yevshev and a team as code owners October 30, 2024 01:55
@graham-chainlink graham-chainlink changed the title feat: support enable disable feeds managers/JD [DPA-1067]: feat: support enable disable feeds managers/JD Oct 30, 2024
@@ -1086,7 +1086,9 @@ func (s *service) Start(ctx context.Context) error {
}
}
} else {
s.connectFeedManager(ctx, mgrs[0], privkey)
if mgrs[0].DisabledAt == nil {
Copy link
Contributor

@patrickhuie19 patrickhuie19 Oct 30, 2024

Choose a reason for hiding this comment

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

Are we asserting logic based on a side effect? DisabledAt reads like it would hold a timestamp as a result of a Disable action. Is there a more direct state variable you can read from? Just from reading, something like Disabled or Enabled would be more appropriate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey patrick, my auto merged kicked in and merged this PR.

But let me explain, so as you have guessed correctly ,the DisabledAt is a timestamp field in feeds manager table that holds a time when user disables a feeds manager, and enabling it will clear this field. This is the only field we have, we did started with a single Enabled field, but we ended up with DisabledAt to be consistent with other services (CLO) that we have mainly for consistency.

Copy link
Contributor

@patrickhuie19 patrickhuie19 left a comment

Choose a reason for hiding this comment

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

LGTM, with one non-blocking comment

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