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

Pindexer remove unused appviews #4910

Merged
merged 3 commits into from
Oct 30, 2024
Merged

Conversation

cronokirby
Copy link
Contributor

Describe your changes

This removes:

  • The Dex AppView
  • The ShieldedPool AppView

The Dex AppView shouldn't be needed, given that we have an app view specific to the dex explorer now.

The shielded pool app view was just an example, which can be removed given the many other examples now.

Other AppViews which should be removed in a later PR after making sure they're not used:

  • the supply AppView
    • superceded by the insights app view, but is known to be used
  • the ibc AppView
    • I think there's ongoing development using this, potentially?

Testing

The other app views should continue to work.

Checklist before requesting a review

  • I have added guiding text to explain how a reviewer should test these changes.

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    indexing only

@hdevalence
Copy link
Member

SGTM

@conorsch conorsch self-requested a review October 30, 2024 16:23
@conorsch
Copy link
Contributor

the supply AppView, superceded by the insights app view, but is known to be used

Where? If we know for sure, some proactive outreach might be warranted. Also happy to just leave it in place if you are.

@conorsch conorsch merged commit 37f604f into main Oct 30, 2024
14 checks passed
@conorsch conorsch deleted the pindexer-remove-unused-appviews branch October 30, 2024 16:39
@cronokirby
Copy link
Contributor Author

https://github.com/radiantcommons/penumbra-token-info uses it ; should be a very simple change

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