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: dao proposals query key #1412

Merged
merged 1 commit into from
Nov 23, 2024
Merged

fix: dao proposals query key #1412

merged 1 commit into from
Nov 23, 2024

Conversation

n0izn0iz
Copy link
Collaborator

without this, if we query the proposals for a teritori dao and the gno query resolves first with nothing, the teritori query will return an empty list

Signed-off-by: Norman Meier <[email protected]>
@n0izn0iz n0izn0iz self-assigned this Nov 22, 2024
Copy link

netlify bot commented Nov 22, 2024

Deploy Preview for teritori-dapp ready!

Name Link
🔨 Latest commit 1117504
🔍 Latest deploy log https://app.netlify.com/sites/teritori-dapp/deploys/6740b4edfc7b6e0007f84ed7
😎 Deploy Preview https://deploy-preview-1412--teritori-dapp.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

netlify bot commented Nov 22, 2024

Deploy Preview for testitori ready!

Name Link
🔨 Latest commit 1117504
🔍 Latest deploy log https://app.netlify.com/sites/testitori/deploys/6740b4edd31e74000739d4d4
😎 Deploy Preview https://deploy-preview-1412--testitori.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.

@n0izn0iz n0izn0iz enabled auto-merge November 22, 2024 18:01
Copy link
Collaborator

@hthieu1110 hthieu1110 left a comment

Choose a reason for hiding this comment

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

as the name of the hook is useDAOProposals which is sth generic, not dedicated to Gno so I suggest:

  • either keep the hook generic name and make all the query keys (string "gno" => networkId) more generic
  • or change the name to useGnoDAOProposal

but not mix both generic name + hardcoded network => quite confusing

@hthieu1110 hthieu1110 dismissed their stale review November 23, 2024 15:03

change from Change requested => Comment

Copy link
Collaborator

@hthieu1110 hthieu1110 left a comment

Choose a reason for hiding this comment

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

as the name of the hook is useDAOProposals which is sth generic, not dedicated to Gno so I suggest:

either keep the hook generic name and make all the query keys (string "gno" => networkId) more generic
or change the name to useGnoDAOProposal
but not mix both generic name + hardcoded network => quite confusing

@n0izn0iz
Copy link
Collaborator Author

I think you misread the diff, the hook is indeed generic, it's just that the gno version is not extracted where the cosmwasm one is

It was a quickfix but I can refacto to improve clarity

@n0izn0iz n0izn0iz disabled auto-merge November 23, 2024 22:18
@n0izn0iz
Copy link
Collaborator Author

merging because this bug is very anoying

@n0izn0iz n0izn0iz closed this Nov 23, 2024
@n0izn0iz n0izn0iz reopened this Nov 23, 2024
@n0izn0iz n0izn0iz merged commit 16b2f01 into main Nov 23, 2024
34 of 35 checks passed
@n0izn0iz n0izn0iz deleted the fix-dao-proposals-query branch November 23, 2024 22:18
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.

3 participants