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

WIP -- COLO Coin Redemption #331

Merged
merged 2 commits into from Feb 15, 2018

Conversation

Projects
None yet
5 participants
@eswarasai
Copy link
Contributor

commented Jan 30, 2018

Description

Added a page to allow users to redeem the COLO coin

Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)
Refers/Fixes

Fixes: #261

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2018

@owocki -- I've wrapped up the pending changes for this task. Please find below steps to test the changes :

  1. Run migrations -- ./manage.py migrate dashboard
  2. Set below local settings values appropriately --
# WEB3
WEB3_HTTP_PROVIDER = 'TODO'

# COLO Coin
COLO_ACCOUNT_ADDRESS = 'TODO'
COLO_ACCOUNT_PRIVATE_KEY = 'TODO
  1. Create COLO CoinRedemption via below management command with the arguments count, network, token_name, contract_address, amount: (number of tokens), expiry_date: (in seconds)
./manage.py create_coin_redemption_shortcodes 5 ropsten COLO 0x2941dEaad71ADb02B944bd38EBce2f1F4C9A62Dc 1 604800

Let me know if there are any modifications/suggestions/issues that are needed to be taken care of. Thanks.


class Command(BaseCommand):

help = 'generates some coin redemption shortcodes'

This comment has been minimized.

Copy link
@mbeacom

mbeacom Feb 10, 2018

Contributor

Can you modify this file to use 4-space indentation?



class CoinRedemption(SuperModel):
shortcode = models.CharField(max_length=255, default='')

This comment has been minimized.

Copy link
@mbeacom

mbeacom Feb 10, 2018

Contributor

Can you modify these additions to use 4-space indentation throughout?

transaction_id = w3.eth.sendRawTransaction(signed.rawTransaction)

crr = CoinRedemptionRequest.objects.create(
coin_redemption=coin,

This comment has been minimized.

Copy link
@mbeacom

mbeacom Feb 10, 2018

Contributor

Can you modify the lines throughout this method to use 4-space indentation?

signed = w3.eth.account.signTransaction(tx, settings.COLO_ACCOUNT_PRIVATE_KEY)
transaction_id = w3.eth.sendRawTransaction(signed.rawTransaction)

crr = CoinRedemptionRequest.objects.create(

This comment has been minimized.

Copy link
@mbeacom

mbeacom Feb 10, 2018

Contributor

You can drop the variable assignment here and simply do: CoinRedemptionRequest.objects.create(

})

signed = w3.eth.account.signTransaction(tx, settings.COLO_ACCOUNT_PRIVATE_KEY)
transaction_id = w3.eth.sendRawTransaction(signed.rawTransaction)

This comment has been minimized.

Copy link
@mbeacom

mbeacom Feb 10, 2018

Contributor

You could transaction_id = w3.eth.sendRawTransaction(signed.rawTransaction).hex() and reference transaction_id in place of calling transaction_id.hex() throughout.

@@ -31,18 +32,22 @@

from app.utils import ellipses, sync_profile
from dashboard.helpers import normalizeURL, process_bounty_changes, process_bounty_details
from dashboard.models import Bounty, BountySyncRequest, Interest, Profile, ProfileSerializer, Subscription, Tip
from dashboard.models import Bounty, BountySyncRequest, Interest, Profile, ProfileSerializer, Subscription, Tip, CoinRedemption, CoinRedemptionRequest

This comment has been minimized.

Copy link
@mbeacom

mbeacom Feb 10, 2018

Contributor

Once syntax errors are resolved throughout the additions, please run isort app/dashboard/views.py from your web repository root directory, so the imports reflect something like:

from dashboard.models import (
    Bounty, BountySyncRequest, CoinRedemption, CoinRedemptionRequest, Interest, Profile, ProfileSerializer,
    Subscription, Tip,
)

and
from web3 import HTTPProvider, Web3

var forwarding_address = $("forwarding_address").value.trim();

// Check for valid checksum address
if (!forwarding_address || !(/^(0x)?[0-9a-f]{40}$/.test(forwarding_address) || /^(0x)?[0-9A-F]{40}$/.test(forwarding_address))) {

This comment has been minimized.

Copy link
@mbeacom

mbeacom Feb 10, 2018

Contributor

Maybe we could use if (!forwarding_address || !web3.isAddress(forwarding_address)) { here?

token_name = models.CharField(max_length=255)
contract_address = models.CharField(max_length=255)
amount = models.IntegerField(default=1)
expires_date = models.DateTimeField()

This comment has been minimized.

Copy link
@mbeacom

mbeacom Feb 10, 2018

Contributor

Per our slack discussion, do you think you can either override save() on this model and self.contract_address = web3.toChecksumAddress(self.contract_address) or perform the toChecksumAddress in the management command to ensure the contract_address is always stored such that it'll succeed during redemption?


class CoinRedemptionRequest(SuperModel):
coin_redemption = models.OneToOneField(CoinRedemption, blank=False, on_delete=models.CASCADE)
ip = models.CharField(max_length=50)

This comment has been minimized.

Copy link
@mbeacom

mbeacom Feb 10, 2018

Contributor

Just a note, there is an IP Address model field type. https://docs.djangoproject.com/en/2.0/ref/models/fields/#genericipaddressfield

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2018

@mbeacom -- I've updated the code as per your comments. Let me know if there's anything else pending. Thanks.

@owocki

This comment has been minimized.

Copy link
Member

commented Feb 12, 2018

@mbeacom last we spoke you were having trouble redeeming the coins.. is that still the case?

@@ -135,4 +135,11 @@ AWS_SECRET_ACCESS_KEY = 'TODO'
S3_REPORT_BUCKET = 'TODO'
S3_REPORT_PREFIX = 'TODO'

# WEB3
WEB3_HTTP_PROVIDER = 'TODO'

This comment has been minimized.

Copy link
@owocki

owocki Feb 12, 2018

Member

could this just default to mainnet.infura.io?

This comment has been minimized.

Copy link
@eswarasai

eswarasai Feb 12, 2018

Author Contributor

@owocki -- Sure. I can go ahead and do that in the following commit. But for Infura, we might probably need the key as well right?

This comment has been minimized.

Copy link
@owocki

owocki Feb 13, 2018

Member

not as of now. in the future, we might need a key

This comment has been minimized.

Copy link
@eswarasai

eswarasai Feb 13, 2018

Author Contributor

@owocki -- I've updated this with the https://mainnet.infura.io now

@mbeacom

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2018

@owocki Testing now with COLO on Ropsten.

WEB3_HTTP_PROVIDER = 'TODO'

# COLO Coin
COLO_ACCOUNT_ADDRESS = 'TODO'

This comment has been minimized.

Copy link
@owocki

owocki Feb 12, 2018

Member

@mbeacom could you PR a related change to the server repo with a new account for this? from there i can fund it with some tokens / eth

expires_date=timezone.now() + timezone.timedelta(seconds=options['expires_date'])
)

print("https://gitcoin.co/coin/redeem/" + cr.shortcode)

This comment has been minimized.

Copy link
@owocki

owocki Feb 12, 2018

Member

awesome!

address = body['address']

try:
coin = CoinRedemption.objects.get(shortcode=shortcode)

This comment has been minimized.

Copy link
@owocki

owocki Feb 12, 2018

Member

using this code... whats to stop me from just using the same shortcode over and over again

This comment has been minimized.

Copy link
@mbeacom

mbeacom Feb 12, 2018

Contributor

^ Agreed. Maybe we should check for the existence of a CoinRedemptionRequest for the coin before performing the transfer.

This comment has been minimized.

Copy link
@eswarasai

eswarasai Feb 12, 2018

Author Contributor

^^Bummer. I've added that check for GET request and totally forgot about the similar issue for the API apart. Let me quickly do that. Thanks for pointing this out 👍

This comment has been minimized.

Copy link
@eswarasai

eswarasai Feb 12, 2018

Author Contributor

@owocki @mbeacom -- This has been taken care of now

@owocki

This comment has been minimized.

Copy link
Member

commented Feb 13, 2018

@eswarasai mind handling the merge conflicts!?

@mbeacom mind taking a pass at this with one more set of 👀 when ur in between the feb integration branch stuff?

@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2018

@mbeacom -- Sorry about the force push. I've resolved the conflicts locally and committed the changes. By the time I saw your commit, I've already pushed the changes. Let me know if there's anything I've missed in the merge. Thanks for your help :)

@mbeacom

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2018

@eswarasai Hate to ask you, but would you mind resolving the conflicts on this PR?

thelostone-mc and others added some commits Jan 24, 2018

redeem coin: base skeleton setup
- mimicked UI + functionality of recieve tip
- added template + route
- supports ether transfer
Eswara Sai
@eswarasai

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2018

@mbeacom -- Done 👍

@owocki

This comment has been minimized.

Copy link
Member

commented Feb 15, 2018

yayayaaya! lets ship this before the craziness of ETHDenver starts :)

@owocki owocki merged commit a770a04 into gitcoinco:master Feb 15, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@owocki owocki removed the in progress label Feb 15, 2018

@owocki

This comment has been minimized.

Copy link
Member

commented Feb 15, 2018

🎉

@PixelantDesign PixelantDesign referenced this pull request May 10, 2018

Closed

test #1135

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.