search

osmosis-labs / isotonic

Smart Contracts for the Lendex Protocol
MIT License
1 stars 0 forks source link

All tokens replaced with being either native or cw20 #108

Closed hashedone closed 2 years ago

hashedone commented 2 years ago

Closes #69

At this point I just changed every interface tokens to accept either native or cw20, but in the contract I basically assert for being native (raise error otherwise).

I need to review it myself before exit out of draft - I am not sure if I didn't substitute too much (some tokens I changed could be actually specifically lendex tokens).

hashedone commented 2 years ago

Also building issue is caused with some problems with multitest package, would take a look tomorrow.

hashedone commented 2 years ago

I left it on purpose - I am pretty much sure that the denom here is actually ltoken itself, anyway - I don't think we want to accept native or cw20 here. I dont't think that this one belongs to this task.

uint commented 2 years ago

I'm looking at the transfer_from fn and it seems the amount: Coin thing is supposed to be in common_token. Then it's converted by an oracle, etc.

https://github.com/confio/isotonic/blob/main/contracts/isotonic-market/src/contract.rs#L592

hashedone commented 2 years ago

Ok, I got rid of this Coin, and it seems to be working, but I am not sure exactly why - I need to tripple check it before merging

uint commented 2 years ago

Okay, I've looked through this and I see two things.

  1. This query returns utils::credit_line::CreditLineResponse, which includes fields with Coin. https://github.com/confio/isotonic/blob/69-cw20-tokens-api/contracts/isotonic-market/src/msg.rs#L94 There's a good reason for that. We want to be able to validate that we got the expected denom from these queries. I think we want to implement our own version of Coin that takes native/cw20 denoms into account.

  2. https://github.com/confio/isotonic/blob/69-cw20-tokens-api/contracts/isotonic-market/src/state.rs#L20 - it would probably make sense to change these as well, make sure we don't have to migrate state in the future when we implement cw20.

hashedone commented 2 years ago

According to 2 - I don't want to do this. Migrating state in the future is not a big issue, but I do think, that accepting cw20 may be slightly more complicated and I want not to go into more complex migrations unless I have an idea about how it would be done. I explicitly changed API only, and this is not exactly an API kind of for raw queries, but those are not so much an issue I guess).