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.

Sign up for GitHub

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

Jump to bottom

Rewards/MyRewards: add etherscan link #1653

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Rewards/MyRewards: add etherscan link #1653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ca4f5e2
Rewards/MyRewards: add etherscan link
e18r Nov 25, 2019
babfc38
Rewards/MyRewards: add etherscan link: get tx hash from past events i…
e18r Nov 26, 2019
66d77c8
Rewards/MyRewards: add etherscan link - store link on per-user basis
e18r Nov 27, 2019
3ab3915
Rewards/MyRewards: add etherscan link - removed GU
e18r Nov 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/rewards/app/components/App/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ class App extends React.Component {
isMyReward: true
})}
claimReward={this.claimReward}
claimHashes={this.props.claims.claimHashes}
/>
) : (
<Overview
Expand Down
74 changes: 48 additions & 26 deletions apps/rewards/app/components/Content/MyRewards.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import PropTypes from 'prop-types'
import React from 'react'
import React, { useState } from 'react'
import styled from 'styled-components'
import {
ContextMenu,
ContextMenuItem,
DataView,
IconCoin,
IconHash,
IconView,
Text,
useTheme,
Expand All @@ -14,48 +15,62 @@ import {
ONE_TIME_DIVIDEND,
RECURRING_DIVIDEND,
ONE_TIME_MERIT,
READY,
CLAIMED,
} from '../../utils/constants'
import { Empty } from '../Card'
import Metrics from './Metrics'
import { useAppState } from '@aragon/api-react'
import { useAppState, useAragonApi, useNetwork } from '@aragon/api-react'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I remove the useAppState hook and import appState from useAragonApi? I've seen it elsewhere but I'm not 100% sure it's the same thing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes 😄

They are the same, and I think I prefer always using useAragonApi

import BigNumber from 'bignumber.js'
import { displayCurrency, getStatus } from '../../utils/helpers'
import { displayCurrency, getStatus, getStatusId } from '../../utils/helpers'

const MyRewards = ({
myRewards,
myMetrics,
viewReward,
claimReward,
claimHashes,
}) => {
const network = useNetwork()
const rewardsEmpty = myRewards.length === 0
const { api } = useAragonApi()
const [ user, setUser ] = useState()
api.accounts().subscribe(accounts => setUser(accounts[0]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the same as const { connectedAccount } = useAragonApi()? See docs here – I think it might be.

Why include the user's address in the claimHashes structure? Why not add transactionHash to the reward directly, so we can get rid of this user logic, and simplify getEtherescanLink with something like:

const link = `https://${networkChunk}etherscan.io/tx/${reward.transactionHash}`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of the way rewards work. You create a reward, and then any token holder can claim it. Since multiple users should be able to claim a reward at any time, we need to track who claimed it. Your approach would only store the last claim, so every user would see the last claim made by any user, instead of their own claim.


const getEtherscanLink = reward => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this use any component state? If not, let's move it outside the definition of MyRewards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does use claimHashes which is a prop. And also network, which is a hook. So not possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer we use useCallback, in that case.

const getEtherescanLink = useCallback(reward => {
  // existing function here
}, [ claimHashes, network ])

Benefits:

  1. The function will only be redefined if claimHashes or network change, rather than being redefined on every render of this function
  2. Code clarity/communication: this helps anyone else looking at this code quickly understand what bits of component state this function relies on (claimHashes and network)

const networkChunk = network.id === 1 ? '' : `${network.type}.`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could have used network.type, but I'm not sure about it's return value for mainnet. If it's mainnet, it's not useful because mainnet.etherscan.io doesn't work. The documentation is rather laconic about it, and I couldn't find code examples. In any case, checking the id seems more robust. Obviously, this hasn't been tested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you test this and check? Does it use the user-configured setting from MetaMask?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I meant by this not having been tested is that it can only be tested on mainnet. Or am I missing something? What setting from Metamask? If you're wondering about the network id, there's no doubt that mainnet is 1.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I'm wondering if useNetwork() responds to the user setting in MetaMask. If it does, then you can set MM to mainnet and then check network.type to see what it looks like.

How does useNetwork() check what network its on, other than using MetaMask? This might be a question for @topocount or @Quazia.

Copy link
Member

@ottodevs ottodevs Dec 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

That's just an interface that gets an updated value for the Aragon's api network observable:
https://github.com/aragon/aragon.js/blob/e4a155ef82420147d4926756861a5bd6cd46f84f/packages/aragon-api/src/index.js#L79

It gets the value from the current provider, it can be MM or other tool injecting the provider.

The answer is yes it responds to the user setting in MetaMask

const claimHash = claimHashes[reward.rewardId][user]
const link = `https://${networkChunk}etherscan.io/tx/${claimHash}`
return link
e18r marked this conversation as resolved.
Show resolved Hide resolved
}

if (rewardsEmpty) {
return <Empty noButton />
}

const renderMenu = (reward) => (
<ContextMenu>
<StyledContextMenuItem
onClick={() => viewReward(reward)}
>
<IconView css={{
marginRight: '11px',
marginBottom: '2px',
}}/>
View
</StyledContextMenuItem>
{(reward.claims < reward.disbursements.length && (reward.disbursements[reward.claims] < Date.now())) && (
<StyledContextMenuItem
onClick={() => claimReward(reward)}
>
<IconCoin css={{
marginRight: '11px',
marginBottom: '2px',
}}/>
Claim
</StyledContextMenuItem>)}
</ContextMenu>
)
const renderMenu = (reward) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could also move this outside of the component, and make it a component. But that's probably out-of-scope for now.

Copy link
Contributor Author

@e18r e18r Nov 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

So renderMenu is used as DataView's renderEntryActions property. According to AragonUI's docs, renderEntryActions gets a function, not a component.
Screenshot from 2019-11-27 12-19-46

Does it still make sense for it to become a functional component and/or be moved outside?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still think it makes sense to move it outside. I guess we can't make it a component right now, though it might be worth opening an issue in the aragonUI repo.

const statusId = getStatusId(reward)
return (
<ContextMenu>
<StyledContextMenuItem onClick={() => viewReward(reward)} >
<StyledIcon Icon={IconView} />
View reward
</StyledContextMenuItem>
{statusId === READY && (
<StyledContextMenuItem onClick={() => claimReward(reward)} >
<StyledIcon Icon={IconCoin} />
Claim
</StyledContextMenuItem>
)}
{statusId === CLAIMED && (
<StyledContextMenuItem href={getEtherscanLink(reward)}>
<StyledIcon Icon={IconHash} />
View on Etherscan
</StyledContextMenuItem>
)}
</ContextMenu>
)
}

return (
<OverviewMain>
Expand Down Expand Up @@ -143,6 +158,13 @@ MyRewards.propTypes = {
myMetrics: PropTypes.arrayOf(PropTypes.object).isRequired,
}

const StyledIcon = ({ Icon }) => (
<Icon style={{
marginRight: '11px',
marginBottom: '2px',
}}/>
)

const OverviewMain = styled.div`
background-color: #f8fcfd;
`
Expand Down
13 changes: 11 additions & 2 deletions apps/rewards/app/store/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,12 @@ import { addressesEqual } from '../utils/web3-utils'
import { INITIALIZATION_TRIGGER } from './'

export const handleEvent = async (state, event, settings) => {
const { event: eventName, returnValues, address: eventAddress, } = event
const {
event: eventName,
returnValues,
address: eventAddress,
transactionHash,
} = event
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we access the network here, rather than in the frontend? We don't want the link to change, depending on which network the user is currently connected to. We want to link them to the URL where they can view the actual transaction, whichever network that happened to be on!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're on one network you won't see any rewards made on another network in the first place, so I think it's safe to access the network from the frontend.

const { vault } = settings

let nextState = { ...state, }
Expand All @@ -25,7 +30,11 @@ export const handleEvent = async (state, event, settings) => {
nextState.isSyncing = false
break
case 'RewardClaimed':
nextState = await onRewardClaimed(nextState, returnValues)
nextState = await onRewardClaimed(
nextState,
returnValues,
transactionHash
)
break
case 'RewardAdded':
nextState = await onRewardAdded(nextState, returnValues, settings)
Expand Down
12 changes: 9 additions & 3 deletions apps/rewards/app/store/reward.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,17 @@ export async function onRewardAdded({ rewards = [], refTokens = [], balances = [

export async function onRewardClaimed(
{ rewards = [], claims = {} },
{ rewardId, claimant }
{ rewardId, claimant },
transactionHash
) {
rewards[rewardId] = await getRewardById(rewardId, claimant)

let { claimsByToken = [], totalClaimsMade = 0 } = claims
let { claimsByToken = [], totalClaimsMade = 0, claimHashes = {}, } = claims

claimHashes[rewardId] = {
...claimHashes[rewardId],
[claimant]: transactionHash
}

const tokenIndex = claimsByToken.findIndex(token => addressesEqual(token.address, rewards[rewardId].rewardToken))

Expand All @@ -43,7 +49,7 @@ export async function onRewardClaimed(

totalClaimsMade = await getTotalClaims()

return { rewards, claims: { claimsByToken, totalClaimsMade } }
return { rewards, claims: { claimsByToken, totalClaimsMade, claimHashes } }

}

Expand Down
3 changes: 3 additions & 0 deletions apps/rewards/app/utils/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,6 @@ export const DISBURSEMENT_UNITS = [
YEARS,
]
export const OTHER = 'Other…'
export const PENDING = Symbol('PENDING')
export const READY = Symbol('READY')
export const CLAIMED = Symbol('CLAIMED')
e18r marked this conversation as resolved.
Show resolved Hide resolved
29 changes: 23 additions & 6 deletions apps/rewards/app/utils/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ import {
ETH_DECIMALS,
ETH_DECIMALS_NUMBER,
RECURRING_DIVIDEND,
PENDING,
READY,
CLAIMED,
} from './constants'
import BigNumber from 'bignumber.js'
import {
Expand Down Expand Up @@ -55,7 +58,7 @@ Status.propTypes = {
time: PropTypes.string,
}

export const getStatus = ({
export const getStatusId = ({
rewardType,
timeClaimed,
endDate,
Expand All @@ -64,16 +67,30 @@ export const getStatus = ({
}) => {
if (rewardType === RECURRING_DIVIDEND) {
if (claims === disbursements.length)
return <Status type="claimed" time={timeClaimed} />
return CLAIMED
if (Date.now() > disbursements[claims].getTime())
return <Status type="ready" />
return <Status type="pending" />
return READY
return PENDING
}
else {
if (timeClaimed > 0)
return <Status type="claimed" time={timeClaimed} />
return CLAIMED
if (Date.now() > endDate)
return <Status type="ready" />
return READY
return PENDING
}
}

export const getStatus = (reward) => {
const statusId = getStatusId(reward)
switch(statusId) {
case PENDING:
return <Status type="pending" />
case READY:
return <Status type="ready" />
case CLAIMED:
return <Status type="claimed" time={reward.timeClaimed} />
default:
return ''
}
}