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

fix: don't throw ipfs errors, use defaults #149

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

wgwz
Copy link
Contributor

@wgwz wgwz commented Sep 12, 2023

Closes: #146

Testing:

  1. create a proposal with an invalid ipfs url for a group policy address i.e.
➜  ~ cat proposal.json 
{
  "@type": "/cosmos.group.v1.MsgSubmitProposal",
  "group_policy_address": "regen1mqevjln5hxv3qgd3c4m5zjeeand5hkc7r33ty82fjukw9shxjh6s6d4umv",
  "proposers": ["regen1fhjpuh6qh6kjsedtmfxrayvmzlknkzde92ky2j"],
  "metadata": "ipfs://CID",
  "messages": [],
  "exec": "EXEC_UNSPECIFIED"
}
  1. submit the proposal via cli i.e.
➜  ~ ~/go/bin/regen tx group submit-proposal proposal.json --keyring-backend test --chain-id test 
  1. Go to the page for the group in the groups-ui, observe that the group page and proposal page loads successfully.

  2. Checkout dev, repeat 1-3, observe the failure.

@wgwz wgwz requested a review from a team September 12, 2023 21:52
@netlify
Copy link

netlify bot commented Sep 12, 2023

Deploy Preview for groups-ui-storybook ready!

Name Link
🔨 Latest commit 685652f
🔍 Latest deploy log https://app.netlify.com/sites/groups-ui-storybook/deploys/6500dd8c8860110008d03e55
😎 Deploy Preview https://deploy-preview-149--groups-ui-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Sep 12, 2023

Deploy Preview for regen-groups-ui ready!

Name Link
🔨 Latest commit 685652f
🔍 Latest deploy log https://app.netlify.com/sites/regen-groups-ui/deploys/6500dd8c6f2a8f0008ed1c54
😎 Deploy Preview https://deploy-preview-149--regen-groups-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

You can also test using the deploy preview connected to mainnet here.

Any ideas why the proposal is not showing up when we hit this error? I think we want to have a fallback in place and show the proposal. It looks like this solves the immediate issue though with the page not loading.

I'm ok with addressing the fallback separately if not a quick fix.

Screenshot from 2023-09-12 15-17-39

@wgwz
Copy link
Contributor Author

wgwz commented Sep 12, 2023

@ryanchristo it was showing up for me when i tested locally, i’ll take a better look but it may just be a separate issue. i’m kind of confused by what’s showing in your logs, unless i’m mistaken i thought the new log statement i added would have shown up there.

oh you know what, maybe it was because i tested the metadata as precisely ipfs://CID whereas the metadata in the proposal Sam added might have had quote characters around it. i’ll look at more and see if that was it, and if so i’ll just do a fix here

@ryanchristo
Copy link
Member

You should also be able to test locally with the proposal on mainnet: http://localhost:5173/6

@ryanchristo
Copy link
Member

You should also be able to test locally with the proposal on mainnet: http://localhost:5173/6

Actually you would hit a CORS error if you did this with the production indexer so nvm.

@wgwz
Copy link
Contributor Author

wgwz commented Sep 12, 2023

> Any ideas why the proposal is not showing up when we hit this error? I think we want to have a fallback in place and show the proposal. It looks like this solves the immediate issue though with the page not loading.

@ryanchristo i think the reason that proposal doesn't show up is because the deploy preview is not connected to the production indexer either, right? the groups-ui deploy previews are connected to the staging indexer only, unless i'm just totally misremembering stuff.

nevermind. that's not the case. duh

@wgwz
Copy link
Contributor Author

wgwz commented Sep 12, 2023

Actually you would hit a CORS error if you did this with the production indexer so nvm.

we can disable CORS temporarily on the server, and that should make it work, and allow better debugging

@wgwz
Copy link
Contributor Author

wgwz commented Sep 12, 2023

it looks like a possible explanation for why the proposals are not showing up here:
https://deploy-preview-149--regen-groups-ui.netlify.app/6
is a bug in the historical proposals fetching.
it looks like this group has two group policies.
TL;DR there may be a bug in the useGroupHistoricalProposals function in this case.
i'm not sure how well i tested this scenario.
it's possible that because one of the indexer API calls returns an empty result set, that we are using that.
for one of the policy addresses we find proposals and for the other policy we get the empty set.
you can see this in the network tab when you have the group page open, and requests filtered to "graphql":

Request/response for first policy:
Screen Shot 2023-09-12 at 7 25 29 PM
Screen Shot 2023-09-12 at 7 25 36 PM

Request/response for second policy:
Screen Shot 2023-09-12 at 7 25 43 PM
Screen Shot 2023-09-12 at 7 25 49 PM

it's fairly obvious that's a bug because we get some historical proposals, so those should be showing.
pretty sure this explains it

Copy link
Member

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

tACK. Sounds like a separate issue. This pr looks good to me.

@wgwz
Copy link
Contributor Author

wgwz commented Sep 13, 2023

Opened an issue for the missing proposals:
#150

@wgwz wgwz merged commit 71ecb09 into dev Sep 13, 2023
8 checks passed
@wgwz wgwz deleted the kyle/146-dont-crash-for-ipfs-errors branch September 13, 2023 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unsupported proposal metadata preventing application use
2 participants