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

Add snap cronjobs #651

Merged
merged 6 commits into from Oct 31, 2022
Merged

Add snap cronjobs #651

merged 6 commits into from Oct 31, 2022

Conversation

FrederikBolding
Copy link
Member

@FrederikBolding FrederikBolding commented Jul 21, 2022

Fixes: #353

Cronjob feature will add new possibility to periodically run specific Snap RPC methods. Cronjob feature is implemented as a new permission with caveats that are used to specify job methods, parameters and schedule using CRON syntax.

For interpretation of a CRON syntax, cron-parser library is added: https://www.npmjs.com/package/cron-parser
cron-parser is added as the dependency to the snap-utils and snap-controllers packages.

New caveat specification is added under the snap-utils package inside cronjob.ts.
Cronjob feature's core is implemented as the Controller inside CronjobController.ts which is part of snap-controllers package.

CronjobController is supposed to be registered in the extension within MetaMask Controller.

Example Cronjob Specification caveat:

{
      expression: '* * * * *',
      request: {
        method: 'exampleMethodOne',
        params: ['p1'],
      },
    }

Cronjob Controller will on its start subscribe to certain Snap events and check for possible cronjobs in order to register or remove them from schedule.
CronjobController will listen for the following events:

  • SnapController:snapInstalled - will add cronjobs and schedule for snap
  • SnapController:snapRemoved - will remove all added/scheduled cronjobs for snap
  • SnapController:snapUpdated - will remove registered jobs and scheduled jobs and add them again according to the newest specification

@david0xd
Copy link
Contributor

Here is the PR that will follow up this one (when merged/released) in order to complete the integration with MetaMask (Flask) Extension: MetaMask/metamask-extension#16239

@david0xd
Copy link
Contributor

After some research I found out that using of a CRON syntax might still be the best (in my opinion).

For many scheduling requirements and cases it's highly possible for a developer to come across the CRON standard.
I found out that the CRON syntax can be very extensive with possibilities for scheduling and since we're using the cron-parser which is quite popular library, we're getting most of it in a reliable way.
It seems to be easy to start working with it once you get some documentation reference. We can put some explanation and few examples of the CRON schedule. I find this tool very useful as well https://crontab.guru/every-2-minutes

Here are some examples of the CRON expressions and their meaning:
*/2 * * * * -> “At every 2nd minute.”
0 0 1 */6 * -> “At 00:00 on day-of-month 1 in every 6th month.”
0 9-17 * * * -> “At minute 0 past every hour from 9 through 17.”
1-59/2 * * * * -> “At every 2nd minute from 1 through 59.”
*/15 * * 1 6 -> “At every 15th minute on Saturday in January.”

There were some ideas for splitting parameters into the simpler format by its meaning, but it seems if the original format is kept, it's more powerful and meaningful for the one who already knows to use it. Since users of this feature would be developers, I do not think they're going to have much of a trouble with this.

If we continue to use pure CRON way of doing the schedule we will have less to maintain and test in the future (avoid potential bugs as well). That way we have a feature out of the box that works well, is widely used and specified which means less concern about its functionality now and later.

@david0xd david0xd marked this pull request as ready for review October 24, 2022 09:05
@david0xd david0xd requested a review from a team as a code owner October 24, 2022 09:05
packages/controllers/src/cronjob/CronjobController.test.ts Outdated Show resolved Hide resolved
packages/controllers/src/cronjob/CronjobController.test.ts Outdated Show resolved Hide resolved
packages/controllers/src/cronjob/CronjobController.test.ts Outdated Show resolved Hide resolved
packages/controllers/src/cronjob/CronjobController.test.ts Outdated Show resolved Hide resolved
packages/controllers/src/cronjob/CronjobController.test.ts Outdated Show resolved Hide resolved
packages/controllers/src/test-utils/controller.ts Outdated Show resolved Hide resolved
packages/utils/src/cronjob.ts Show resolved Hide resolved
packages/utils/src/cronjob.ts Show resolved Hide resolved
packages/controllers/src/snaps/endowments/cronjob.test.ts Outdated Show resolved Hide resolved
packages/utils/src/cronjob.test.ts Show resolved Hide resolved
Comment on lines 142 to 172
it('successfully parses cronjob expression that is provided as an object', () => {
const cronjobExpression = {
minute: '*',
hour: '*',
dayOfMonth: '*',
month: '*',
dayOfWeek: '*',
};
expect(() => parseCronExpression(cronjobExpression)).not.toThrow();
});

it('successfully parses cronjob expression that is provided as a string', () => {
const cronjobExpression = '* * * * *';
expect(() => parseCronExpression(cronjobExpression)).not.toThrow();
});
Copy link
Member

Choose a reason for hiding this comment

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

Rather than asserting that these don't throw, maybe check that the return value is correct.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a few different tests to ensure that the object properties are put in the right place too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've removed throw assertion and added couple of things that check the properties of the object returned which is from an external library cron-parser. These methods will only work if the parsing is successful, otherwise it will throw an error which would break the test. I hope that's ok now.

@FrederikBolding FrederikBolding changed the title Cronjob feature Add snap cronjobs Oct 31, 2022
@FrederikBolding FrederikBolding force-pushed the fb/cronjob-feature branch 2 times, most recently from 2697cd5 to e462748 Compare October 31, 2022 10:35
this.updateJobLastRunState(job.id, Date.now());
this.#messenger.call('SnapController:handleRequest', {
snapId: job.snapId,
origin: '',
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this? Or should it maybe be something like 'cron'?

Copy link
Member Author

Choose a reason for hiding this comment

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

We currently use empty string for transaction insights as well - It is currently required to have a value by the snap executor. We should perhaps figure out a way to only require it for certain handlers? Out of scope for this PR imo.

FrederikBolding and others added 6 commits October 31, 2022 12:47
Fix build error

Fix test

Add part of cronjob caveat implementation

Register cronjob caveat

Add more caveat validations and tests

Fix imports after rebase

Fix conflict-merge issue after rebase

Fix validation functions build issue

Add configuration for caveat mapper

Update CronjobService and add tests

Add some more unit tests for CronjobService

Update CronjobService to become CronjobController

Add some refactoring and new features for the CronjobController

Refactor and clean up

More refactor and tests

Add missing docstring

Improve test coverage for CronjobController

Improve test coverage for utils

Improve test coverage for CronjobController

Fix execution-environments commands

Fix jest threshold after rebase

Refactor directory structure

Add support for object-like cron expressions

Fix and rework a couple of things for structural expressions

Add some refactoring & test improvements

Code refactoring

Add way to unregister all snaps on controller destroy method call

Change timers and snapIds to be private with getters

Remove eslint ignore instructions where they are not needed anymore

Redesign the test with daily timeout check (schedule skip)

Refactor unit tests
@FrederikBolding FrederikBolding merged commit da0e3ab into main Oct 31, 2022
@FrederikBolding FrederikBolding deleted the fb/cronjob-feature branch October 31, 2022 12:01
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.

Cronjob permission (Q4)
4 participants