search

celestiaorg / celestia-app

Celestia consensus node
https://celestiaorg.github.io/celestia-app/
Apache License 2.0
335 stars 275 forks source link

Fix tokenfilter tests #3238

Closed cmwaters closed 3 months ago

cmwaters commented 5 months ago

Summary

Right now our token filter tests have been commented out:

https://github.com/celestiaorg/celestia-app/blob/897b448cb8d2b655743bf1e786f6823775468964/test/tokenfilter/tokenfilter_test.go#L135-L141

The reason being that the app version isn't passed in the header (so the tests simply panic).

We should fix this and re-enable the tests. Currently they rely on the testing package provided by the ibc-go repo. It's possible to suggest the fix there. The problem is that we would want the changes backported to the v6.x branch which is no longer supported (only v7 and v8 are supported). I would prefer not to have to fork another repo so the options would be to:

  1. Rewrite the tests using interchaintest (the docker image based one that strangelove maintain)
  2. Upgrade to v8 in v2 and submit the patch that supports different app versions
rootulp commented 5 months ago

Thanks for creating the issue! I lean slightly towards option 2 because I found it difficult to use interchaintest when writing ICA tests. We only technically need to bump to IBC v7+ if we adopt RIM but I think it's a good idea to bump to IBC v8 in celestia-app v2 because IBC v6 is now unsupported.

cmwaters commented 5 months ago

Yeah I suggested v8 without looking into much detail around the decision but purely basing it on the fact that the later the version, the longer we have before it's no longer maintained :)

rootulp commented 5 months ago

@cmwaters @ninabarbakadze and I had a discussion and we're leaning towards adopting interchaintest to add end-to-end tests for ICA and PFM. In the long-term we'd like to use one testing library so now it seems like we're leaning towards option 1 over option 2.

rootulp commented 5 months ago

We may be able to fix these tests if the fix for https://github.com/celestiaorg/celestia-app/issues/3137 applies to the /v6 branch.

ninabarbakadze commented 5 months ago

the fix in #3137 is specifically for tokenfilter tests

rootulp commented 5 months ago

Got it, that's what I thought. I wanted to link https://github.com/celestiaorg/celestia-app/issues/3137 to this issue b/c they can both be closed if we fix upstream ibc-go/testing on the /v6 branch.

ninabarbakadze commented 5 months ago

i think this issue should also cover updating to the new version that includes the fix and enabling the tokenfilter test before we close it

ninabarbakadze commented 5 months ago

i'm happy to finish working on this all the way @rootulp

rootulp commented 4 months ago

Nice to have b/c we're testing tokenfilter via the end-to-end tests covered in https://github.com/celestiaorg/celestia-app/issues/3241

rootulp commented 4 months ago

Unblocked because we upgraded to ibc-go v6.3.1 with Nina's fix