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

Ensure gauges can only be created for assets that exist on-chain #530

Closed
ValarDragon opened this issue Oct 14, 2021 · 3 comments · Fixed by #855
Closed

Ensure gauges can only be created for assets that exist on-chain #530

ValarDragon opened this issue Oct 14, 2021 · 3 comments · Fixed by #855
Assignees

Comments

@ValarDragon
Copy link
Member

Currently gauges seem to be creatable for assets that don't exist on-chain. This doesn't really pose any efficiency problems, but we should nonetheless eliminate this. (If nothing else, just for integrators sanity lol)

Here is an instance for a gauge for the denom "Stake".

    {
      "id": "244",
      "is_perpetual": false,
      "distribute_to": {
        "lock_query_type": "ByDuration",
        "denom": "stake",
        "duration": "604800s",
        "timestamp": "2021-03-16T17:57:55Z"
      },
      "coins": [
        {
          "denom": "ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2",
          "amount": "1000"
        }
      ],
      "start_time": "2021-07-01T20:36:45Z",
      "num_epochs_paid_over": "5",
      "filled_epochs": "0",
      "distributed_coins": []
    },

This can be fixed by making the CreateGauge logic in x/incentives check that the denomination exists in the bank keeper on the chain, somewhere.

@jeebster
Copy link
Contributor

jeebster commented Nov 3, 2021

@ValarDragon I've been working on this but am a bit uncertain about a few things with regards to denomination existence logic:

  1. Should the check be performed against the lockup.QueryCondition Denom or sdk.Coin Denom? I would think the latter but you mention the lockup.QueryCondition.Denom ("stake") in the issue description.

  2. Should supply or metadata be referenced for validating asset existence on-chain? I noticed there will be a HasSupply() method available so I would think supply is the standard reference for checking existence, however, technically an asset can exist on chain with no supply (after supply runs out). This seems like an edge case since supplies shouldn't run out for quite some time, but I figured I'd defer to you on the preferred methodology to verify asset existence

Either way, I'm seeing lots of test failures checking both lockup.QueryCondition Denom and sdk.Coin Denom so it would be nice to have some clarity on the preferred approach

@ValarDragon
Copy link
Member Author

ValarDragon commented Nov 5, 2021

The check should basically be:

bankkeeper.HasSupply(lockup.QueryCondition.Denom)

We want to upgrade to SDK v0.44 ASAP, so probs best to wait on that. (Or can build it in a separate branch using Jacob's PR)

I don't really understand what it means for an asset to exist on chain with no supply. Is this an edge case in the current supply code or smth?

@jeebster
Copy link
Contributor

jeebster commented Nov 5, 2021

Sounds good, thanks for clarifying this - will wait until this functionality is available via SDK version upgrade to continue with implementation

I haven't taken a deep look into the SDK supply code, but I'm making an assumption here that an asset is not removed - for history/transparency - if the supply ultimately reaches zero (though I guess this is theoretically impossible); it's quite possible I'm incorrect about this expectation. Anyways, this is why I figured it may be safer to check asset presence via attributes other than the supply, but seeing the check of supply as you mentioned appears the way to proceed

@daniel-farina daniel-farina added this to the 2022 - January Milestone milestone Dec 2, 2021
@daniel-farina daniel-farina removed this from the 2022-01 Milestone milestone Dec 24, 2021
@ValarDragon ValarDragon moved this from 🕒 Todo to ❌ Blocked in Osmosis Chain Development Jan 24, 2022
@ValarDragon ValarDragon moved this from ❌ Blocked to 🕒 Todo in Osmosis Chain Development Feb 2, 2022
@AlpinYukseloglu AlpinYukseloglu moved this from 🕒 Todo to 🏃 In Progress in Osmosis Chain Development Feb 7, 2022
Repository owner moved this from 🏃 In Progress to ✅ Done in Osmosis Chain Development Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants