OpenZeppelin / gsn-site

The Ethereum Gas Station Network Alliance Landing Page and Tools
MIT License
12 stars 15 forks source link

Query RelayHub for recipient balance #6

Closed frangio closed 5 years ago

frangio commented 5 years ago

Fixes the following issue that is part of #1:

the dapp tool assumes that the recipient implements a getRecipientBalance method, which may not always be the case.

I implemented this using the GraphQL stuff that is available in the project.

asselstine commented 5 years ago

Hi @frangio! I missed you at EthBerlin.

I think you could optimize the PR if you replaced recipientQuery in the RecipientForm with your recipientBalanceQuery. The only thing we needed from recipientQuery was the balance, which is no longer used. We can get relayHubAddress from the props. Saves some calls. The RecipientBalance component looks good, but I don't think it's needed here.

frangio commented 5 years ago

We can get relayHubAddress from the props.

From what I see, the only way it's available in the props right now is as a result of the recipientQuery. The indirection is necessary because we need to ask the recipient what RelayHub it's using.

Am I getting this right?

asselstine commented 5 years ago

Yes! My mistake, in the wee hours I was seeing your component props as the RecipientForm props.

So: your component makes sense! The only thing I'd additionally do then is to remove the balance: getRecipientBalance from the recipientQuery if no one else is using it. May as well remove the dead code.

frangio commented 5 years ago

Done.

Should we also add the subscription logic to RecipientBalance? I suppose this is what refreshes the information periodically?

https://github.com/OpenZeppelin/gsn-site/blob/96d7168e61c0bd909ef389342c228241d51bcc85/lib/components/RecipientForm.jsx#L69-L82

asselstine commented 5 years ago

Yup; that's what keeps the data fresh! If you add a corresponding snippet it will be updated in realtime.

frangio commented 5 years ago

Cool. It should be ready now.

frangio commented 5 years ago

Thanks for reviewing @asselstine!