topology-foundation / topchain-node

MIT License
2 stars 7 forks source link

impr: Add filtering in KVStore iterator #11

Open JanLewDev opened 2 months ago

JanLewDev commented 2 months ago

Filter out the cancelled and expired deals to avoid these cases:

am.keeper.IterateDeals(ctx, func(deal types.Deal) bool {
    switch deal.Status {
    case types.Deal_EXPIRED:
        return false
    case types.Deal_CANCELLED:
        return false
    ...
juanmiguelar commented 2 months ago

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Hello! My name is Juan! :D I am from Costa Rica!

I'm interested in the topology project! Since I saw it in X, I am currently reading the white paper and I am already in the telegram channel. My telegram ID is: @iamjuandev

I have about 4 years of Go programming experience, I'm trying to get into the open source community, looking for good first issues to contribute and learn from.

How I plan on tackling this issue

I can begin to understand the current context of the code and become familiar with the repository. Identify the business logic for filtering. And adding unit tests.

JanLewDev commented 2 months ago

Sure, go ahead!

juanmiguelar commented 2 months ago

Hello! @JanLewDev

I have some questions for beginners!!

I've been looking at the code and it looks like the change is already there. (1*) image

Or do you mean we should merge the cases into one? case types.Deal_EXPIRED, types.Deal_CANCELLED: return false

Or get rid of those cases? Just delete them?

(1*) - https://github.com/topology-foundation/topchain-node/blame/3c2cc3e4d3c92e3638e2f932028fe8c75136e9d2/x/subscription/module/module.go#L160

JanLewDev commented 2 months ago

We want a more efficient way of avoiding the iteration over expired and cancelled deals. Currently, the iterator returns all deals, and there are possibly a lot of deals that we don't have to iterate through.

func (k Keeper) IterateDeals(ctx sdk.Context, shouldBreak func(deal types.Deal) bool) {
    storeAdapter := runtime.KVStoreAdapter(k.storeService.OpenKVStore(ctx))
    iterator := prefix.NewStore(storeAdapter, types.KeyPrefix(types.DealKeyPrefix)).Iterator(nil, nil)
    defer iterator.Close()

The cases in switch shouldn't be needed, we need research to see if they can be ignored at the iterator level. If you have more questions, you can join our tg and discord