Rewards/MyRewards: add etherscan link

[e18r]

Fixes #1631

Context

I'm working on adding a new context menu option for claimed rewards, which contains a link to the transaction's hash on Etherscan.

Possible solution using approach # 4 (see below)

Create a front end trigger that allows the store to be aware of the current user, and only retrieve the claims that pertain to that user. For each such claim, build an Etherscan link and store it in every reward object that has been claimed at least once. Replace older claims so that only the latest claim of a specific reward gets stored.

Historical summary of considered approaches

Approach 1: store the claim hash in the front end state

When the user claims a reward, I subscribed to the observable and stored the transaction hash in the claimHashes object:

Then, when the user clicks on the "View on Etherscan" button, the getEtherscanLink function is called, which constructs the URL by drawing from claimHashes:

@chadoh's objection

Given that claimHashes is being stored in the component's state, it goes away on browser refresh. We not only need it to remain across refreshes, but also across different browsers. In other words, we want claimHashes to be persistent.

Approach 2: store the claim hash in the contract

In the context of a dapp, persistence means storing stuff in the contract. So I went ahead and modified the contract to include a claimHashes object, a getter and a setter. Take a look at them here. It's a work in progress; I still need to add tests and the front-end integration. Also, I'll add the object to the Reward struct instead of it being a separate entity.

Possible objections

This is a big and unexpected change to the contract, just to add a link to Etherscan. So a few objections can be raised:

• It might not be worth the effort. Let's not implement an Etherscan link for now.
• Let's implement it, but without modifying the contract. The user can only have access to it right after it's claimed.
• Let's store the claim hashes on IPFS (how to do that?)
• There's a way to retrieve the transaction hash from the blockchain each time without the need to store anything. This would be the preferred approach, I think, if it was possible. But is it?

Approach 3: retrieve the claim hash from past blockchain events

After getting feedback from @ottodevs, @Quazia, @stellarmagnet and @chadoh, I have a clearer picture of the situation: there's no need to persist something that is alread being persisted on the blockchain. All I have to do is retrieve the the transaction hash from past blockchain events at the store level. Thank you all for this!

Solution

On the store, a new data structure, claimHashes, is created. It's an object of objects, storing claim hashes on a per-reward and per-user basis. Whenever a claim event is retrieved from the blockchain, the transaction hash is taken from the event information, and the reward ID and user address are taken from the return values of the claimReward contract function.

Then, on the front-end, the currentOn the store, a new data structure, claimHashes, is created. It's an object of objects, storing claim hashes on a per-reward and per-user basis. Whenever a claim event is retrieved from the blockchain, the transaction hash is taken from the event information, and the reward ID and user address are taken from the return values of the claimReward contract function.

Then, on the front-end, the current user address is retrieved by calling AragonAPI's accounts function, and it's used, along with the contextual reward ID, to build the apropriate etherscan link. user address is retrieved by calling AragonAPI's accounts function, and it's used, along with the contextual reward ID, to build the apropriate etherscan link.

Approach 4: only retrieve the claim information for the current user

After discussing approach # 3 with @chadoh, we realized that there's no need to create a data structure to hold the transaction IDs of every claimed disbursement for every reward, since we're doing this on a specific user's front end and we only care about this user's last claim for each reward. Instead, at the store level, we can create a Etherscan link by looking at claim events where the claimant corresponds to the current user. Since there's only one etherscan link per reward, it can be directly stored as a field in each reward object in the rewards data structure.

If it's not possible to obtain the current user at the store level, it can be obtained via a front end trigger.

Also, this can be easily extended to include the claims for every disbursement of a specific reward, in order to allow users to have a more historical way of looking at the claims they have made.

Note that this won't work if the dapp user is switched. This is not the only case in which this happens, so we probably need a general way of handling with user switching events in our apps.

Finally, I think adding this Etherscan link can start a pattern of adding Etherscan links in our apps, which makes sense because they allow dapp users to have a less trusted interaction with our dapps, and also to diagnose potential problems with transactions.

[chadoh]

I guess an alternative would be:

this.setState({ claimHashes: {
  ...this.state.claimHashes,
  [reward.rewardId]: claimHash,
}})

I'm not sure how I feel about my formatting; hopefully eslint has opinions that we can defer to.

I don't think my suggestion or the way you wrote this read more clearly. I have questions either way!

• Do you think claimedHashes is a more accurate name? These are past-tense claimed, right?
• claimHashes is initialized to an empty object, and it's only updated when a reward is claimed. If the user refreshes the page, this link will go away. Am I understanding correctly? Is that what we want?

[e18r]

Do you think claimedHashes is a more accurate name? These are past-tense claimed, right?

You're the native speaker, but the hashes themselves aren't the thing being claimed. Instead, the hashes refer to claims. So the claim in claimHashes is a noun rather than a verb. Still, if it doesn't sound right to you, let's consider other options: claimsHashes seems more accurate to me because there are many claims, but it sounds bad. Any other ideas?

[chadoh]

I'm leaning toward "get rid of claimHashes altogether," as mentioned here and here. We don't have to stick with the current structure of rewards that we have now; we can add transactionHash or, better yet, etherscanUrl to each reward, and simplify all the logic we have here.

[e18r]

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.

[chadoh]

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

[e18r]

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.

[chadoh]

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.

[ottodevs]

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

[chadoh]

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

[e18r]

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

[chadoh]

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)

[chadoh]

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

[e18r]

So renderMenu is used as DataView's renderEntryActions property. According to AragonUI's docs, renderEntryActions gets a function, not a component.

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

[chadoh]

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.

[e18r]

After what was discussed in today's meeting, I decided to stop my current work and ask around if I'm in the right direction. I'm updating this PR's description to reflect the current state.

[stellarmagnet]

I don't think we should modify the contract at this stage for this feature. Additionally, we need to figure out how important it is to keep churning on this. Nobody user requested it (iirc), so my thoughts are we close this PR and keep it as a future backlog item for Rewards.

Any time a contract change is being considered, buy-in should be received from the original contract authors and myself before commencing development work.

[e18r]

@stellarmagnet There's no need to modify the contract for this, as @ottodevs kindly explained to me yesterday. Thank you for the clarification.

[chadoh]

There's a way to retrieve the transaction hash from the blockchain each time without the need to storing anything. This would be the preferred approach, I think, if it was possible. But is it?

Yes, I believe there is! I think a quick pairing session might be the best way to get you started with this; check your calendar invites 😄

[e18r]

Hey @chadoh, I think I got it. Thank you so much though!

[e18r]

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.

[chadoh]

Yes 😄

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

[chadoh]

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}`

[e18r]

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.

[chadoh]

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!

[e18r]

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.

[chadoh]

See above comments – I think we can get probably get rid of claimHashes and clean all of this up quite a bit!

[ottodevs]

The code seems good overall, I am still a bit confused about the use of claimHashes, and I wonder if that is really needed, I also see there are some comments addressed by Chad than can lead to a cleaner code, like removing useAppState, replacing the accounts subscription by the api-react hook, and other details...

I will revisit this after those changes are performed, so I can properly test locally and do a final review.

Thanks for the great work Emilio!