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

Token dependent on single wallet address #107

Open
mbeacom opened this issue Sep 21, 2018 · 4 comments

Comments

Projects
2 participants
@mbeacom
Copy link

commented Sep 21, 2018

Discussion

Following a discussion with @octavioamu, it occurred to me that Token is highly dependent on a single wallet address. For instance, if a user switches their preferred_payout_address at any point, all kudos previously received will not display anywhere on their profile or be able to link back to the original user on the Gitcoin platform.

Originally, we discussed introducing a Wallet intermediate model that relates to Profile and Token, with the option to set a default flag on one wallet at a time.

If we introduce something like an economy.models.Wallet similar to:

class Wallet(SuperModel):
    """Define the Wallet data schema for profiles."""

    address = models.CharField(max_length=255, db_index=True)
    is_default = models.BooleanField(default=False)
    profile = models.ForeignKey('dashboard.Profile', on_delete=models.CASCADE, related_name='wallets')
    wallet_users = models.ManyToManyField('dashboard.Profile')

and updating Token to include wallet = ForeignKey('economy.Wallet', on_delete=models.SET_NULL, related_name='tokens')

We could then update the Profile model to migrate the preferred_payout_address field to new Wallet objects, delete the field, and add a property on Profile like:

    @property
    def preferred_payout_address(self):
        """Return the preferred payout address for this Profile."""
        try:
            return self.wallets.get(is_default=True).address
        except Wallet.DoesNotExist:
            return ''

This should greatly reduce the initial lift throughout the codebase from preferred_payout_address to default Wallet.

This allows us to perform prefetch/select_relates on the appropriate models and limit our queries while also ensuring we can always display historic kudos and transactions for a Profile.

Additionally, this allows a dev to simply query the profile and access profile.wallets.tokens in views and templates while easily paginating/lazy loading the results.

If you'd like, I can raise this issue on gitcoinco/web and deploy it first to simplify your migration if we opt to go this route.

Consideration during implementation:

Possible "attack vector" with the current implementation... If a user wants to display another user's kudos on their profile, they could simply:
CURRENT IMP: Update their preferred_payout_address to another user's address from the top of the leaderboard.

Any thoughts on this?

cc @owocki @jasonrhaas

@mbeacom mbeacom added the backend label Sep 21, 2018

@jasonrhaas

This comment has been minimized.

Copy link
Collaborator

commented Sep 21, 2018

I like the idea. If you want to make that change in the master branch, that would be great. I can pull it in then and add the FK for the Kudos Token.

As far as the attack vector: We don't allow users to add an arbitrary address from the Profile. They can only add an address that's in their MetaMask account.

However, regarding the "Indirect Tips" receive, if the user uses someone else's address and then sets it as their preferred_payout_address, it would essentially cause them to take over the account. This can be prevented by only allowing the Receiver to use an address that is provided by MetaMask (much like how the Profile works).

@jasonrhaas jasonrhaas added this to Backlog in Kudos via automation Sep 21, 2018

@mbeacom

This comment has been minimized.

Copy link
Author

commented Sep 24, 2018

@jasonrhaas Pushing out the PR today. I'll @ mention you on the PR when it's up.

Regarding the attack vector, if you browse to https://gitcoin.co/settings/account - you can update your preferred_payout_address to anything there. That's why I was saying in the event we were to use preferred_payout_address, this could be a concern. If we're relying on that field and only that field, this would be a valid concern since the majority of the existing queries are checking preferred_payout_address to determine Kudos to be rendered on the user's profile page.

@jasonrhaas

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2018

I'm totally good with not using the preferred_payout_address directly. Octavio and I have gone back and forth on this, and ultimately it came down to that we needed a broader solution for the whole application (which is what you are merging in).

I think this change will solve it for Kudos and the Gitcoin app as a whole so that a user can have multiple wallets associated with their profile.

@jasonrhaas

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2018

Waiting on this PR to get this in before Kudos launch -- gitcoinco/web#2276

@jasonrhaas jasonrhaas moved this from To Do to In progress in Kudos Oct 18, 2018

@jasonrhaas jasonrhaas moved this from In progress to To Do in Kudos Oct 19, 2018

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.