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

feat: basket msg server #686

Closed
wants to merge 47 commits into from
Closed

feat: basket msg server #686

wants to merge 47 commits into from

Conversation

technicallyty
Copy link
Contributor

@technicallyty technicallyty commented Jan 12, 2022

Description

Adds baskets

Basket Addresses

Baskets now derive an address upon creation. This allows us to use our existing ecocredit query endpoints without having to add basket specific queries. Need to know the balance of a basket? call the existing Balance endpoint with the basket's address.

This may not be the most ideal solution as a potential attack vector would be to send credits to this address using the Send method, effectively bypassing the filter. we could give basket addresses a custom prefix regenbskt1v32.... and have Send actively check for this. not too sure if that's ideal, so curious to hear thoughts on it.

Basket Coin Denom

Currently, I use the basket name as the token denom alone. This is placeholder for a better solution. The name is used as the coin denom for the basket's token.

To avoid being confused with other tokens on regen network, i think basket tokens should have a prefix before the basket name as their token name. Similar wBTC for wrapped bitcoin, we could do something like bsktFooBasket for a token from FooBasket. This would be good for UX, especially in the case where a troublemaker might make a basket called OSMO and try to fool people with the basket token.

Questions

  • Should we allow basket's to have no filter?
  • TakeFromBasket only returns tradable amounts, but this method can also result in credits being retired. should we include a RetiredAmount in the response message?
  • Say someone submits a TakeFromBasket tx with a requested amount of 3.123456789, and one of the batches in the basket has a precision of 6. What do we do in this case?
  • Should we cache the total number of credits in the basket table in ORM? this could save us a lot of computation time in TakeFromBasket where we iterate over all the coins, and fail if there aren't enough for the swap. cacheing the amount would allow us to fail early if a user requests more than the basket has.
  • Should the basket curator be allowed to pick if picking is disabled?
  • Should filters have depth limits?
  • What is the basket display name for? how should it be validated?
  • should we allow basket exponents of 0? this would result in a 1:1 of basket token -> basket credit.
  • currently i loop over all the batches in the basket, then sort by start date. is this the best/only way?

Stuff that still needs to be implemented:

  • filter that allows you to simply set the filter to batches no older than 3 years ago
  • Events
  • methods to update basket criteria
  • basket creation fee gov param

Closes: #634
Closes: #683


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@ryanchristo
Copy link
Member

ryanchristo commented Jan 22, 2022

Some notes from our call in case you missed something:

Create basket:

  • Basket criteria should be enforced.
  • Maybe display name shouldn't be included in request. Maybe auto-generated from name and exponent (i.e. E6, E9). Display name would go into denom metadata. Bank denom metadata needs to be set.

Add Basket:

  • Using depth adds complexity. Filter should be a single filter, not an array. Remove "for" loop for filters. Each case (or and and) would have their own "for" loop. In case of "and", you want to check all. In case of "or", you only need one match.
  • This is horizontal depth, you want to have vertical depth. Depth limit should be enforced in validate criteria. We can use depth of 3 for now and revisit in separate issue later.

TakeFromBasket:

  • Integrate ORM

Filter:

  • Credit Types should be removed from the filter, and a basket should have a credit type as a top level field. this avoids complexity with baskets that could contain credits with varying precisions.

@robert-zaremba
Copy link
Collaborator

ON HOLD : blocked by orm code gen work in SDK.

@technicallyty
Copy link
Contributor Author

closing as we can just port #732

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ecocredit: sendCredits method fails on empty amount Implement Baskets MsgServer
3 participants