Buyback and Burn #1318
Buyback and Burn #1318
Conversation
- fork only necessary osmomath files - fork cl pool types and price calc func
…oracle.proto and regenerate go bindings
Removes redundant 'stride' and 'v1beta1' segments from API paths to make them cleaner and more consistent - /icqoracle/price - /icqoracle/prices - /icqoracle/params
Removes 'stride/x/' prefix from amino message names to maintain consistent naming for: - MsgRegisterTokenPriceQuery - MsgRemoveTokenPrice
| tokenPrice.SpotPrice = math.LegacyZeroDec() | ||
| tokenPrice.UpdatedAt = time.Time{} | ||
| tokenPrice.QueryInProgress = false |
There was a problem hiding this comment.
Sam:
i don't think you want to override these
The way I think about this function is, if we (hypothetically) ever had to fork the chain, we would want to export the full state and then initialize the new chain with that state. So in this world, I don't think we would want to mess with the fields.
Wdyt?
There was a problem hiding this comment.
Assaf:
This function is only called on genesis, where we don't even have IBC set up. In my mind these values are definitely stale at this point.
I guess there's no real reason to reset SpotPrice & UpdatedAt because there's also PriceExpirationTimeoutSec to make sure they are not used when stale. As for QueryInProgress, there's definitely no IBC at this point so I feel confident we can reset it and prevent potential issues down the road.
There was a problem hiding this comment.
In my mind these values are definitely stale at this point.
Agreed with that
However, this function is also relevant when we do state exported testnets when we start up a forked chain with the current state. In this case, we would notice there are some differences (which we could visually verify are fine, but still kind of annoying to deal with).
I think it makes more sense to just keep values as what they are. The genesis comes from the JSON which is hard coded.In practice it will never be used, but in theory, if we had specific values set on the JSON, that probably means we want to use them (otherwise, we would just scrape them ourselves)
There was a problem hiding this comment.
okay, and what about QueryInProgress?
There was a problem hiding this comment.
same deal, my personal opinion is that if we had it set to true for some reason in genesis.json, we should keep it like that (otherwise we would have manually set it to false)
| // TokenPrice stores latest price data for a token | ||
| message TokenPrice { | ||
| // Token denom on Stride | ||
| string base_denom = 1; | ||
| // Quote denom on Stride | ||
| string quote_denom = 2; | ||
| // Token denom on Osmosis | ||
| string osmosis_base_denom = 3; | ||
| // Quote denom on Osmosis | ||
| string osmosis_quote_denom = 4; | ||
| // Pool ID on Osmosis | ||
| string osmosis_pool_id = 5; |
There was a problem hiding this comment.
@sampocs sampocs Dec 17, 2024
i wonder if it's worth adding a human readable name to this too? or maybe we can just enrich it with the full denom trace during the RPC query
Thinking being that if we do this with all our host zone tokens, then we'll have a bunch of IBC denom's and it gets pretty clunky to try and manually inspect things
@assafmo assafmo Dec 17, 2024
yeah i don't like the fields here too, i'll try to think of a better way to represent this
There was a problem hiding this comment.
i think internally we should just keep it as is cause there's already so much on the struct. But I think we could add a query that figures out the denom traces
| // fcfsBidHandler handles bids for First Come First Serve auctions | ||
| func fcfsBidHandler(ctx sdk.Context, k Keeper, auction *types.Auction, bid *types.MsgPlaceBid) error { | ||
| // Get token amount being auctioned off | ||
| moduleAddr := k.accountKeeper.GetModuleAddress(types.ModuleName) |
There was a problem hiding this comment.
sampocs Dec 17, 2024
should we use a module account per auction? it would make it easier to support multiple auctions with the same denom
assafmo Dec 17, 2024
Yes, but not sure that we want to do it now. I've thought about it a bit, and for the MVP this will complicate the way we send fees to be auctioned off. For each fee token we'll have to map on-chain what auction gets what percentage.
Lmk what you think, I'll do what you decide because I don't have a strong feeling here
There was a problem hiding this comment.
can you say more on why we would have to figure out percentages?
The way I was thinking about it was that we would make a module account using the auction name that gets created when you create an auction. And then in stakeibc or when we send fees, we would do something like:
for denom in reward collector balance:
auctionName = f"stakeibc-fees-{denom}"
auction, found = k.GetAuction(auctionName)
if not found:
continue
k.SendCoins(from: rewardCollector, to: auction.Address)
That said, to your point, I don't think it has to be in the MVP, but if it's simple enough like that, I think we should do it
There was a problem hiding this comment.
My thinking is that it's possible there will be multiple auctions with the same selling token. E.g. maybe in the future we'll want to create 2 auctions for ATOM, one Dutch and the other English, and send 80% of ATOM to the Dutch auction and 20% to the English auction. To implement something like that we can add a priority field to auction and then have stakeibc calculate the percentages from the priorities. E.g. priorities of (1,2,3) translate to percentages (1/6,2/6,3/6).
I think with any of these approaches (all in one account, each auction has an account, each auction has an account + multiple auctions can use same selling denom) we could always migrate to a different approach later on, so I'm not too worried here.
Let's just decide what we want for the MVP and move on.
There was a problem hiding this comment.
Imo, even in that world of split percentages, it should be the onus of the calling module (i.e. stakeibc) to determine those splits ahead of time - so not something the auction module has to take care of. I also think these two are the same: "each auction has an account" and "each auction has an account + multiple auctions can use same selling denom" cause if you have a different account, the denom doesn't matter.
I'll defer to you here since you're heads more in this / you have a better sense of how much work it would be to split accounts. My mental model with stuff like this is that I find small changes that you know for sure you'll want to make are often better done during the v0 since you're heads most into it and it's a little easier to add it now than, say, 6 months from now where you have to bring context back.
So if (a) you're aligned this is the best solution, (b) it would be quick to implement and (c) it doesn't increase the testing complexity too much, then I would say probably makes sense to add it. Otherwise, I think punting it is fine. Defer to you!
to share code between msg validation and genesis validation
| func TokenPriceQueryKey(baseDenom, quoteDenom, poolId string) []byte { | ||
| return []byte(fmt.Sprintf("%s|%s|%s", baseDenom, quoteDenom, poolId)) | ||
| } |
There was a problem hiding this comment.
should this be TokenPriceKey instead to line up with the keepers and struct type?
| func TokenPriceByDenomKey(baseDenom string) []byte { | ||
| return []byte(baseDenom) | ||
| } |
There was a problem hiding this comment.
I think you can remove this and just do the []byte(TokenPrice.BaseDenom) in the keepers
- Introduces v25 upgrade handler with ICQOracle parameter initialization - Adds new module store upgrades for icqoracle, strdburner, and auction Initial ICQOracle parameters: - 5 min update and expiration intervals - 2 min ICQ timeout - Osmosis chain and connection configurations
No description provided.