Skip to content

Conversation

apollo-sturdy
Copy link
Contributor

No description provided.

@apollo-sturdy apollo-sturdy requested review from grod220 and piobab June 5, 2023 16:02
@apollo-sturdy apollo-sturdy force-pushed the feat/multi-incentives branch from 797477b to 59aa76a Compare June 6, 2023 15:04
@apollo-sturdy apollo-sturdy force-pushed the dev/multi-incentives branch from aa37564 to 9c23e56 Compare June 6, 2023 15:07
/// Returns an iterator over all asset incentives, with optional pagination.
/// Caller should make sure that if start_after_incentive_denom is supplied, then
/// start_after_collateral_denom is also supplied.
pub fn paginate_asset_incentives(
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is quite complicated to follow.. What if we simplify an API to paginate incentives per requested collateral? This way we give control to the caller and he can prepare separate msgs to claim rewards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess really it's up to frontend devs what they prefer. The downside of your suggestion is that they would need to know all collaterals that the user has incentives for. I could imagine that this is hard to keep track of. They can't just query Red Bank since the user can remove collateral and still have unclaimed rewards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@piobab what do you think? Personally I think current method is better so backend doesn't need to keep track of state

denom: collateral_denom.to_string(),
},
)?;
let market: red_bank::Market = deps.querier.query_wasm_smart(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm analyzing this now and if incentives is set for market which is not set in redbank it could fail the whole tx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Should we verify that market exists in Red Bank in ExecuteMsg::SetIncentive? It's not possible to remove a market in red bank is it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it is not possible to remove. IMO we should verify in SetIncentive that collateral exists in Red Bank.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@piobab piobab self-requested a review June 14, 2023 11:51
@apollo-sturdy apollo-sturdy merged commit 199efc5 into feat/multi-incentives Jun 15, 2023
@apollo-sturdy apollo-sturdy deleted the dev/multi-incentives branch June 15, 2023 17:34
larry0x pushed a commit that referenced this pull request Oct 7, 2023
* Clear empty accounts.

* Update schema.

* Add tests. Cleanup.

* Improve msg.

* Update CM with rc. New deployment.
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.

4 participants