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

Integrate PollingController mixin with GasFeeController #1673

Merged
merged 7 commits into from
Sep 29, 2023
Merged

Integrate PollingController mixin with GasFeeController #1673

merged 7 commits into from
Sep 29, 2023

Conversation

adonesky1
Copy link
Contributor

@adonesky1 adonesky1 commented Sep 13, 2023 •
edited
Loading

Integrates recently introduced PollingController mixin with GasFeeController. Leaves old polling pattern in pace for now so as to not force a ton of breaking changes for mobile, but with intention to activate new pattern in both clients ASAP.

Addresses: https://github.com/MetaMask/MetaMask-planning/issues/1314

@adonesky1 adonesky1 marked this pull request as ready for review September 18, 2023 16:46
@adonesky1 adonesky1 requested a review from a team as a code owner September 18, 2023 16:46
@Gudahtt
Copy link
Member

Gudahtt commented Sep 18, 2023

Generally this looks good! Will give it a more detailed review later

@adonesky1 adonesky1 force-pushed the networkclientid-polling branch 4 times, most recently from 0ef971b to 41ccfc8 Compare September 20, 2023 02:15
@socket-security
Copy link

socket-security bot commented Sep 21, 2023 •
edited
Loading

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@Gudahtt
Copy link
Member

Gudahtt commented Sep 21, 2023

I suspect that CI will not let us use the new package in the gas fee controller package until we merge and release it separately

Gudahtt
Gudahtt reviewed Sep 21, 2023
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Looks great! Maybe we can move the new package into a separate PR and get that merged and released to unblock the gas fee controller changes.

packages/polling-controller/src/PollingController.test.ts Outdated Show resolved Hide resolved
packages/polling-controller/src/PollingController.ts Outdated Show resolved Hide resolved
packages/polling-controller/src/PollingController.ts Outdated Show resolved Hide resolved
packages/polling-controller/src/PollingController.ts Outdated Show resolved Hide resolved
packages/polling-controller/src/PollingController.ts Outdated Show resolved Hide resolved
@shanejonas shanejonas mentioned this pull request Sep 22, 2023
Merged
shanejonas added a commit that referenced this pull request Sep 26, 2023
@shanejonas @adonesky1
Added polling-controller (#1703)
9698b0a 
## Explanation

> Originally was in #1673 but
pulled out to get this in on its own.

Adds an abstract class (currently named `PollingController`. The start
and stop methods are parameterized by `networkClientId`'s and
`pollingToken`'s and polling intervals are stored in class variables by
`chainId` so that multiple `networkClients`/`chainIds` can poll
simultaneously. `executePoll` is an abstract method to be implemented by
the controller itself so that this pattern could be generalized and be
agnostic to what is being executed on the polling interval.


## References
Related to #1673
Related to https://github.com/MetaMask/MetaMask-planning/issues/1314

---------

Co-authored-by: Alex Donesky <adonesky@gmail.com>
@adonesky1 adonesky1 changed the title Integrate PollingController Mixing with GasFeeController Integrate PollingController mixin with GasFeeController Sep 28, 2023
jiexi
jiexi previously approved these changes Sep 28, 2023
View reviewed changes
@adonesky1 adonesky1 merged commit fd03dc6 into main Sep 29, 2023
107 checks passed
@adonesky1 adonesky1 deleted the networkclientid-polling branch September 29, 2023 15:30
@legobeat legobeat mentioned this pull request Oct 2, 2023
Merged
3 tasks
@legobeat
Copy link
Contributor

legobeat commented Oct 2, 2023

@adonesky1 @shanejonas:

adonesky1 added a commit that referenced this pull request Oct 2, 2023
@legobeat @adonesky1
fix(gas-fee-controller): add missing @metamask/polling-controller (#1748
94175cd 
)

## Explanation

Adds missing dependency introduced in #1673

## References

- Follow-up to: #1673 

## Changelog


## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've highlighted breaking changes using the "BREAKING" category
above as appropriate

---------

Co-authored-by: Alex <adonesky@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
@shanejonas @adonesky1 @MajorLift
Added polling-controller (#1703)
f85cc4d 
## Explanation

> Originally was in #1673 but
pulled out to get this in on its own.

Adds an abstract class (currently named `PollingController`. The start
and stop methods are parameterized by `networkClientId`'s and
`pollingToken`'s and polling intervals are stored in class variables by
`chainId` so that multiple `networkClients`/`chainIds` can poll
simultaneously. `executePoll` is an abstract method to be implemented by
the controller itself so that this pattern could be generalized and be
agnostic to what is being executed on the polling interval.


## References
Related to #1673
Related to MetaMask/MetaMask-planning#1314

---------

Co-authored-by: Alex Donesky <adonesky@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
@adonesky1 @MajorLift
Integrate PollingController mixin with GasFeeController (#1673)
6b88f78 
Integrates recently introduced [`PollingController`
mixin](#1736) with
`GasFeeController`. Leaves old polling pattern in pace for now so as to
not force a ton of breaking changes for mobile, but with intention to
activate new pattern in both clients ASAP.

Addresses: MetaMask/MetaMask-planning#1314
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
@legobeat @adonesky1 @MajorLift
fix(gas-fee-controller): add missing @metamask/polling-controller (#1748
3ed5af7 
)

## Explanation

Adds missing dependency introduced in #1673

## References

- Follow-up to: #1673 

## Changelog


## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've highlighted breaking changes using the "BREAKING" category
above as appropriate

---------

Co-authored-by: Alex <adonesky@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
@shanejonas @adonesky1 @MajorLift
Added polling-controller (#1703)
9b2f6e7 
## Explanation

> Originally was in #1673 but
pulled out to get this in on its own.

Adds an abstract class (currently named `PollingController`. The start
and stop methods are parameterized by `networkClientId`'s and
`pollingToken`'s and polling intervals are stored in class variables by
`chainId` so that multiple `networkClients`/`chainIds` can poll
simultaneously. `executePoll` is an abstract method to be implemented by
the controller itself so that this pattern could be generalized and be
agnostic to what is being executed on the polling interval.


## References
Related to #1673
Related to MetaMask/MetaMask-planning#1314

---------

Co-authored-by: Alex Donesky <adonesky@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
@adonesky1 @MajorLift
Integrate PollingController mixin with GasFeeController (#1673)
e88538f 
Integrates recently introduced [`PollingController`
mixin](#1736) with
`GasFeeController`. Leaves old polling pattern in pace for now so as to
not force a ton of breaking changes for mobile, but with intention to
activate new pattern in both clients ASAP.

Addresses: MetaMask/MetaMask-planning#1314
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
@legobeat @adonesky1 @MajorLift
fix(gas-fee-controller): add missing @metamask/polling-controller (#1748
9ad9a28 
)

## Explanation

Adds missing dependency introduced in #1673

## References

- Follow-up to: #1673 

## Changelog


## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've highlighted breaking changes using the "BREAKING" category
above as appropriate

---------

Co-authored-by: Alex <adonesky@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 12, 2023
@legobeat @adonesky1 @MajorLift
fix(gas-fee-controller): add missing @metamask/polling-controller (#1748
07f21f1 
)

## Explanation

Adds missing dependency introduced in #1673

## References

- Follow-up to: #1673 

## Changelog


## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've highlighted breaking changes using the "BREAKING" category
above as appropriate

---------

Co-authored-by: Alex <adonesky@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shanejonas shanejonas shanejonas approved these changes

@mcmire mcmire Awaiting requested review from mcmire

@BelfordZ BelfordZ Awaiting requested review from BelfordZ

@Gudahtt Gudahtt Awaiting requested review from Gudahtt

@jiexi jiexi Awaiting requested review from jiexi

Assignees
No one assigned
Labels
team-wallet-api-platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@adonesky1 @Gudahtt @jiexi @legobeat @shanejonas @BelfordZ