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 management command to track Kudos purchases #4198

Merged
merged 2 commits into from
Mar 4, 2020

Conversation

santteegt
Copy link
Contributor

@santteegt santteegt commented Apr 13, 2019

Description

Gitcoin team members want to track kudos purchases by recording revenue in the database. So, a management command to track Kudos purchases is implemented to track ETH and token transaction from a specified account address, while a cron job is setup to run the command once an hour.

Checklist
Affected core subsystem(s)
  • revenue
Refers/Fixes

Refs: #4122

Testing and Sign-off

Before testing new features please create an Etherscan API Key on https://etherscan.io/apis and set its value on ETHERSCAN_API_KEY env variable.

Contributor
  • Read and followed the Contributor Guidelines
  • Tested all changes locally
  • Verified existing functionality
  • Ran make test and everything passed!
Reviewer
  • Affirm contributor guidelines have been followed and requested changes made
  • CI tests and linting pass
  • No conflicts (migrations, files, etc)
  • Regression tested against staging or local deployment
Funder
  • Validated requested changes were made to specification
  • Bounty payout released to the contributor

@codecov
Copy link

codecov bot commented Apr 13, 2019

Codecov Report

Merging #4198 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4198   +/-   ##
======================================
  Coverage    30.3%   30.3%           
======================================
  Files         249     249           
  Lines       21470   21470           
  Branches     3115    3115           
======================================
  Hits         6506    6506           
  Misses      14677   14677           
  Partials      287     287

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2558f7...5e919f3. Read the comment docs.

scripts/crontab Outdated
@@ -19,6 +19,7 @@ PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/us
## KUDOS
*/15 3,10,17 * * * cd gitcoin/coin; bash scripts/run_management_command_if_not_already_running.bash sync_kudos mainnet opensea --catchup >> /var/log/gitcoin/sync_kudos_catchup.log 2>&1
*/10 4,11,18 * * * cd gitcoin/coin; bash scripts/run_management_command_if_not_already_running.bash sync_kudos mainnet opensea --start 1 >> /var/log/gitcoin/sync_kudos_all.log 2>&1
0 */1 * * * cd gitcoin/coin; bash scripts/run_management_command_if_not_already_running.bash kudos_revenue mainnet --account-address 0xdb282cee382244e05dd226c8809d2405b76fbdc9
Copy link
Contributor

Choose a reason for hiding this comment

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

*1 == * right?

Copy link
Contributor Author

@santteegt santteegt Apr 15, 2019

Choose a reason for hiding this comment

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

logger.info(f"ETH Tx Records were found. Continuing from block {params['startblock']}")

params['action'] = 'txlist'
call_etherscan_api(network, params)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to call call_etherscan_api() twice? why not just once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Etherscan API uses two separate methods to retrieve ETH transactions (txlist) and ERC-20 token transactions (tokentx)

@santteegt santteegt force-pushed the track-kudos-purchases branch from 9608d37 to 3ecfb9c Compare April 15, 2019 04:33
@owocki
Copy link
Contributor

owocki commented May 29, 2019

this LGTM. @danlipert look ok to you?

@PixelantDesign
Copy link
Contributor

bumping this @danlipert ^^

@danlipert
Copy link
Contributor

@santteegt Can you add some tests for this? Thanks!

@santteegt santteegt force-pushed the track-kudos-purchases branch from 3ecfb9c to c06be7d Compare June 14, 2019 06:06
@santteegt
Copy link
Contributor Author

Hi @danlipert,

I've just updated my PR to consider your review comments

ETHERSCAN_API_KEY = env('ETHERSCAN_API_KEY', default='')

# Kudos revenue account
KUDOS_REVENUE_ACCOUNT_ADDRESS = env('KUDOS_REVENUE_ACCOUNT_ADDRESS', default='0xdb282cee382244e05dd226c8809d2405b76fbdc9')
Copy link
Contributor

Choose a reason for hiding this comment

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

@owocki is this the correct address (and is it the same on rinkeby, or is there even a rinkeby equivalent?)

Copy link
Contributor

Choose a reason for hiding this comment

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

no rinkeby equiv; this is the right addr yes

@danlipert
Copy link
Contributor

@santteegt looking great - I'm wondering if you can test the management command directly and actually inspect the resulting DGP objects that are created after running to ensure they match whats on the blockchain. Also, how are the tests returning data properly even though no API key is set? 🤔

@santteegt
Copy link
Contributor Author

Hi @danlipert,

I've finally added a test case that directly performs a call to the management command. Regarding the empty API key during tests, Etherscan has a limited quota to call their API for free without an API_KEY

@danlipert
Copy link
Contributor

@owocki how is this looking to you?

Copy link
Contributor

@owocki owocki left a comment

Choose a reason for hiding this comment

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

@danlipert LGTM; well have to get an etherscan api key b4 deploy tho!

ETHERSCAN_API_KEY = env('ETHERSCAN_API_KEY', default='')

# Kudos revenue account
KUDOS_REVENUE_ACCOUNT_ADDRESS = env('KUDOS_REVENUE_ACCOUNT_ADDRESS', default='0xdb282cee382244e05dd226c8809d2405b76fbdc9')
Copy link
Contributor

Choose a reason for hiding this comment

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

no rinkeby equiv; this is the right addr yes

@danlipert
Copy link
Contributor

API Key is in place for when this gets merged 👍

@danlipert
Copy link
Contributor

@santteegt we are almost able to get this in, but the automated tests are failing because of the reliance on Etherscan. Can you mock those out so that our automated tests don't depend on Etherscan's availability or whether they block the free requests in the future? Thanks!

@santteegt
Copy link
Contributor Author

@danlipert sure! I'll try to get this done ASAP

@thelostone-mc
Copy link
Member

@santteegt any update on this . ?

Add management command to track Kudos purchases
@santteegt santteegt force-pushed the track-kudos-purchases branch from 2c4cb73 to c2558f7 Compare December 16, 2019 21:28
@santteegt
Copy link
Contributor Author

Hi @thelostone-mc,

Sorry for the late PR submission. I just updated it for you to review

@thelostone-mc thelostone-mc merged commit ac45c7c into gitcoinco:master Mar 4, 2020
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.

6 participants