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

PoP stats service #172

Merged
merged 5 commits into from
Aug 30, 2024
Merged

PoP stats service #172

merged 5 commits into from
Aug 30, 2024

Conversation

gabmontes
Copy link
Contributor

@gabmontes gabmontes commented Jul 9, 2024

Summary

This PR adds a service that will collect all the PoP payout transactions, summarize and report to Absinthe.

See the README file for details.

Related to #170.

Changes

Add everything to the /popstats folder. No additional changes to the repo.

@gabmontes gabmontes self-assigned this Jul 9, 2024
@gabmontes gabmontes force-pushed the pop-stats branch 2 times, most recently from 837e7f5 to 28cc77b Compare August 21, 2024 22:25
@gabmontes gabmontes changed the title Initial PoP stats service poof-of-concept PoP stats service Aug 21, 2024
@gabmontes gabmontes requested a review from a team August 22, 2024 15:22
@gabmontes gabmontes marked this pull request as ready for review August 22, 2024 15:28
@gabmontes gabmontes requested a review from a team as a code owner August 22, 2024 15:28
Copy link
Contributor

@joshuasing joshuasing left a comment

Choose a reason for hiding this comment

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

Looks good. I have left a few comments.

There is also a PNPM workspace already setup under the web/ directory, with ESLint and Prettier already setup. What do you think of moving this to web/packages/pop-stats/? I think this would help keep everything tidier and more consistent.

popstats/Dockerfile Outdated Show resolved Hide resolved
popstats/Dockerfile Show resolved Hide resolved
popstats/README.md Outdated Show resolved Hide resolved
popstats/README.md Outdated Show resolved Hide resolved
popstats/package.json Show resolved Hide resolved
@joshuasing joshuasing added type: enhancement This improves existing functionality size: L This change is large (+/- <500) area: pop-stats This is a change to the pop-stats package labels Aug 22, 2024
@gabmontes
Copy link
Contributor Author

@joshuasing the /web folder is focused on the web PoP miner, UI and WASM code. This is a different process I think it should belong to its own folder.

What we should do is to update its configuration to match the standards in regards to linting and formatting we use in all the other JavaScript repos at Bloq and Hemi.

@gabmontes gabmontes requested a review from joshuasing August 22, 2024 17:12
Copy link
Contributor

@joshuasing joshuasing left a comment

Choose a reason for hiding this comment

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

Would you mind adding popstats/ and yourself to the .github/CODEOWNERS file?

@gabmontes gabmontes requested a review from joshuasing August 22, 2024 17:34
popstats/README.md Outdated Show resolved Hide resolved
popstats/README.md Outdated Show resolved Hide resolved
@gabmontes gabmontes requested a review from gndelia August 22, 2024 19:14
@gabmontes gabmontes enabled auto-merge (squash) August 23, 2024 13:22
Copy link
Contributor

@joshuasing joshuasing left a comment

Choose a reason for hiding this comment

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

Looks good, a few things I missed on my first pass. Will any GitHub Actions be added for this service?

The node.yml file already builds the packages in web/, a separate job could be added there that builds this module.

I assume the Dockerfile will also need to be built for releases, would you like me to make a pull request to add building this to the release.yml action once this has been merged?

.github/CODEOWNERS Outdated Show resolved Hide resolved
popstats/Dockerfile Outdated Show resolved Hide resolved
popstats/README.md Show resolved Hide resolved
popstats/package.json Show resolved Hide resolved
popstats/.nvmrc Outdated Show resolved Hide resolved
@gabmontes
Copy link
Contributor Author

@joshuasing GitHub Actions to automatically build the Docker image could be added later. I will define that with @dhidalgX and send a PR or ask you for help. Thanks for pointing that out!

@gabmontes gabmontes requested a review from joshuasing August 26, 2024 17:38
@gabmontes gabmontes merged commit 7445bc3 into main Aug 30, 2024
6 checks passed
@gabmontes gabmontes deleted the pop-stats branch August 30, 2024 15:02
web3cryptoguy pushed a commit to web3cryptoguy/heminetwork that referenced this pull request Nov 1, 2024
* Add PoP stats service

* Improve Dockerfile, scripts and docs

* Add popstats folder code owner

* Improve docs

Co-authored-by: Gonzalo D'Elia <[email protected]>

---------

Co-authored-by: Gonzalo D'Elia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: pop-stats This is a change to the pop-stats package size: L This change is large (+/- <500) type: enhancement This improves existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants