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 cron job to sync pledge signers #3505

Merged
merged 9 commits into from
Jul 3, 2023
Merged

Add cron job to sync pledge signers #3505

merged 9 commits into from
Jul 3, 2023

Conversation

hyphenized
Copy link
Contributor

@hyphenized hyphenized commented Jun 28, 2023

Implemented a new workflow to account for addresses that could've been dropped during the sync with Galxe. This workflow runs every 3 days and synchronizes addresses over a 4 day sliding window, two of which are from the last sync.

Closes #2678

Latest build: extension-builds-3505 (as of Mon, 03 Jul 2023 22:05:59 GMT).

@hyphenized hyphenized requested a review from a team as a code owner June 28, 2023 17:53
This script should not have it's dependencies checked during the build workflow
@@ -0,0 +1,116 @@
// @ts-check
/* eslint-disable no-console, no-await-in-loop, import/no-unresolved */
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of killing import/no-unresolved, let's have the lint job run yarn install in this package as well as the top level. That job is meant to hit the whole codebase, so we should let it do proper lint checks.

no-await-in-loop and related things that airbnb bans due to regenerator-runtime concerns/functional programming concerns we may want to disable generally for stuff in .github/workflows that is meant to be more scripty. Can discuss that for later though.

Either way, please always drop a comment explaining why linting rules are being disabled.

Comment on lines 78 to 98
const payload = {
operationName: "credentialItems",
query: `
mutation credentialItems($credId: ID!, $operation: Operation!, $items: [String!]!)
{
credentialItems(input: {
credId: $credId
operation: $operation
items: $items
})
{
name
}
}
`,
variables: {
credId: "194531900883902464",
operation: "APPEND",
items: batch,
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder about one thing. 🤔 If we add a new workflow, shouldn't we remove this part from here? Currently, part of the code is duplicated in two places.

https://github.com/tahowallet/taho.xyz/blob/main/manifesto-dapp-backend/functions/src/index.ts#L325-L356

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, forgot to submit this comment. We need both because the other one is dropping addresses at random intervals.


on:
schedule:
- cron: "0 0 */3 * *"
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we also add a workflow_dispatch trigger so we can fire it off manually from the Actions tab?

import admin from "firebase-admin"
import fetch from "node-fetch"

/* eslint-disable prefer-destructuring */
Copy link
Contributor

Choose a reason for hiding this comment

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

But why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Brain fart, this isn't using Webpack 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

Lordy I forgot that was even a thing with Webpack. Love Webpack! <_<

throw new Error("Unable to sync with galxe")
}

await wait(3000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we get rate limit information from Galxe, or is this just to cross our fingers and hope not to topple their server over?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do but it's unlikely that we will get rate limited by Galxe, this is more of a safety measure against the random removal of synced addresses


console.log("Retrieved:", allDocs.length, "addresses")

await wait(1500)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need that when hitting our own Firebase stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately when testing this, trying to pull in too many records or pull records too fast errors out. That was the case with firebase API access so I assumed the same would hold true with the admin-sdk

Copy link
Contributor

Choose a reason for hiding this comment

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

Do those errors give us backoff information by any chance? Not really a blocker, just would be nice to go as hard as they'll let us lol.

@@ -0,0 +1,12 @@
{
"name": "@tallyho/taho-extension-cron-jobs",
Copy link
Contributor

Choose a reason for hiding this comment

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

Also let's maybe rename this dir + package. This isn't a generic cron jobs package, this is specifically a pledge sync job right?. We can do another subdir, but IMO just renaming cron to pledge-signer-sync is fine.

Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

One question about one of the non-secrets being used. Going to set up the rest.

.github/workflows/pledge-signer-sync/pledge-sync.js Outdated Show resolved Hide resolved
@Shadowfiend
Copy link
Contributor

Setting up secrets + getting this merged is on the list today.

Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

All right overall this looks good, and I believe I have the setup correct. Will merge now and then try to kick the action off from the Actions tab.

@Shadowfiend Shadowfiend merged commit df74ca6 into main Jul 3, 2023
@Shadowfiend Shadowfiend deleted the add-sync-cron branch July 3, 2023 22:02
@Shadowfiend Shadowfiend mentioned this pull request Jul 6, 2023
kkosiorowska pushed a commit that referenced this pull request Jul 10, 2023
## Highlights

- An updated Taho token list! This will start auto-updating Soon™.

## What's Changed
* Update Taho token list reference by @kkosiorowska in
#3506
* Don't run e2e tests when workflow is triggered by push of tag by
@michalinacienciala in #3514
* Fix throwing NFT filters panel by @jagodarybacka in
#3513
* Use gas price from 0x quote to setup swap transaction by
@jagodarybacka in #3516
* v0.40.0 by @hyphenized in
#3507
* Add cron job to sync pledge signers by @hyphenized in
#3505
* Fix invalid workflow file by @hyphenized in
#3519
* Remove workflow trigger used for testing by @hyphenized in
#3520
* Fix migration for cleaning account balances by @jagodarybacka in
#3527
* v0.40.1 with op swaps gas price hotfix by @Shadowfiend in
#3518
* v0.40.2 - accounts migration fix by @jagodarybacka in
#3529


**Full Changelog**:
v0.40.2...v0.41.0

Latest build:
[extension-builds-3531](https://github.com/tahowallet/extension/suites/14093546910/artifacts/788185513)
(as of Thu, 06 Jul 2023 02:05:27 GMT).
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.

Galxe Web3pledge credential - missing addresses
3 participants