MrToph / goostew

MIT License
48 stars 4 forks source link

Goo Stew Improvement Proposal 1 #3

Open Philogy opened 1 year ago

Philogy commented 1 year ago

Purpose

Purpose of this proposal is to discuss the proposed changes before beginning with their implementation.

Current State

The current implementation of Goo Stew is a nice improvement over vanilla behavior of holding and managing Gobblers / Goo. However the current implementation has a couple issues:

Useful additional features

Proposed Redesign - Overview

  1. Make the GooStew both an ERC1155 and ERC20 contract, ERC20 methods affect direct ibGOO and ERC1155 the wrapped gobblers
  2. Add a multicall method
  3. Separate the deposit method into separate depositGobblers and depositGoo methods, users can still call them both in 1 tx via multicall
  4. Track Gobbler ibGOO rewards on a per-account rather than per-group / per-Gobbler basis
  5. Use Sushi Masterchef accounting logic to ensure that ibGOO tokens do not necessarily have to be issued upon wrapped Gobbler transfers
  6. Free Balance Accounting of Goo

Proposal Design - ERC20 / ERC1155 Duality

Since the ERC20 and ERC1155 standards do not have overlapping methods or events, they can be implemented in the same contract without making methods partially incompatible with their standards. It is suggested that ibGOO is no longer transferable using ERC1155 methods, this will simplify the contract's logic while also limitting potential confusion in interfacing applications.

Proposal Design - Gobbler Reward Accounting

Account virtual ibGOO accruing to accounts via a signed integer reward debt variable (currently represented by gobblerStakingMap[i].lastIndex), that is increased upon deposits and decreased upon withdrawals / transfers. This allows rewards to be continiously accounted for without having to realize them.

Look at Sushiswap's MasterChef V2 contract for a comparable implementation.

Proposal Design - Efficient Wrapped Gobbler Ownership Tracking

Track wrapped gobbler ownership using the same architecture popularized by Azuki in their ERC721A implementation, only requiring a constant amount of storage slot updates for the issuing of an arbitrary amount of tokens with consecutive IDs.

The issue with consecutive token IDs is that the IDs of Gobblers being deposited will in most cases not be consecutive. To ensure proper tracking of the Gobbler ownership a wrapped ID to Gobbler ID mapping needs to be maintained. Due to the limited range of Gobbler IDs (1 to 10k) the Gobbler ID of consecutive wrapped tokens can be packed together in the same slot. Since a Gobbler ID only requires 14-bits (2^14 = 16384 > 10k) up to 18 Gobblers can be tracked in the mapping while only updating a single storage slot. On average it'll be less than that due to slots already being partially filled from other deposits

Proposal Design - Free Balance Accounting of Goo

While not a commonly used term I like to refer to Uniswap V2 Liquidity Pool styled token accounting as "free balance accounting". Specifically I use this to refer to token accounting whereby tokens that are to be used / withdrawn from a contract need to be transferred to it directly first and are discerend from already deposited assets via a reserve value. The "free balance" is therefore the difference between the contract's total balance and it's explicit reserve.

In the case of Goo in GooStew a reserve may be tracked to discern what part of the virtual Goo is eligible to be withdrawn / deposited although it's likely more efficient to have the free balance be the contract's Goo token balance and the reserve is the virtual Goo balance.

There are several benefits to applying this pattern here:

  1. Integrating utility contracts don't have to call transferFrom twice just to deposit users' assets, they can transferFrom once from a calling user directly to the contract and then call the methods it wants to use
  2. Separating core business logic from token withdrawal / deposit logic simplifies individual methods
  3. Makes methods source/destination agnostic, meaning they don't have to care where the required tokens are coming from / going to allowing for more general and composable individual methods

To ensure that EOAs can still directly interact with the contract safely it is essential the contract has a multicall method so that users can bundle together the necessary methods.

MrToph commented 1 year ago

Hi, thanks for the descriptive writeup. I agree with the issues you mentioned and my initial idea was to just have a Router contract for the extra functionality and an ERC20 wrapper for the ERC1155 goo, this would keep the core contract with its logic as tiny as it is now. Agree, that a claim and view-balance function(uint256[] stakingIds, address owner) should be implemented.

My 2 cents but you are of course free to fork and do whatever you want.

  1. Make the GooStew both an ERC1155 and ERC20 contract, ERC20 methods affect direct ibGOO and ERC1155 the wrapped gobblers

agree, fungible ERC1155 tokens are just not supported enough, easier to do an additional ERC20 or do it all in this contract.

  1. Separate the deposit method into separate depositGobblers and depositGoo methods, users can still call them both in 1 tx via multicall
  2. Add a multicall method

makes sense if you don't like the router idea and keeping the main contract small.

  1. Track Gobbler ibGOO rewards on a per-account rather than per-group / per-Gobbler basis

I guess we would remove the staking NFT this way and keep track of the gobbler ids per account. I like this, then let's just completely remove ERC1155 and only make it an ERC20Permit

  1. Use Sushi Masterchef accounting logic to ensure that ibGOO tokens do not necessarily have to be issued upon wrapped Gobbler transfers

not sure what wrapped gobbler transfers are. imo, minting shares whenever you move gobblers or call claim is fine, the debt/amount struct requires two storage reads instead of a single lastIndex.

  1. Free Balance Accounting of Goo

I don't think it's really necessary or gives any advantages if you do everything in a single contract.

I'd do the following changes:

Philogy commented 1 year ago

Why remove ERC1155 altogether, being able to directly transfer / trade deposited Gobblers seems desirable?

When it comes to accounting I think the total emissions multiple of each account and the reward debt could likely fit in one slot. Always issuing ibGoo is not that efficient as you need to update 3 slots: balance, total supply and reward index. Either way wouldn't you have to keep track of total emissions multiple on a per-account basis?

The "free balance accounting" while not strictly necessary is simpler than I probably made it sound. The pattern is there to simplify the implementation of a mint from goo method together with making integrations simpler.

MrToph commented 1 year ago

Why remove ERC1155 altogether, being able to directly transfer / trade deposited Gobblers seems desirable?

Listing a gobbler on a market place means you need to withdraw it from the protocol. I see how being able to trade a deposited gobbler NFT is nice because then you don't miss out on goo production. (Then again, having deposited gobblers taken off the market might be an even better side-effect for the ecosystem. I haven't decided on what's better yet.) But doesn't this contradict the idea of doing a per-account accounting? Would you issue a single wrapped gobbler NFT per account and update it when they deposit more?

Always issuing ibGoo is not that efficient as you need to update 3 slots: balance, total supply and reward index.

Yes, although the totalSupply is only updated once per block (as it's a lazy mint).

The only difference to your approach is that instead of increasing the balance, you increase the accrued field in the mapping. If you can put (index, accrued) into a single slot, you might save an sload but it seems less elegant to me. (you'd need to change _transfer to look at both _balance and [account].accrued and decrease them both.) Personally, I think it's on the wrong side of optimization <> elegance / readability

Either way wouldn't you have to keep track of total emissions multiple on a per-account basis?

Yes, right now we track total emissions multiple on a per staking ID basis.

Philogy commented 1 year ago

But doesn't this contradict the idea of doing a per-account accounting?

What I'm trying to suggest is wrapping every deposited Gobbler as an individual ERC1155 token. What I meant with per-account accounting is tracking the rewards that accounts accrue rather than Gobbler groups as has been done so far.

(you'd need to change _transfer to look at both _balance and [account].accrued and decrease them both.)

I don't see why you'd need to update accruing ibGOO when you're just transferring ibGOO and not wrapped Gobblers?

Overall I'm realizing that I may not be explaining ideas properly, might be easier to just implement some of what I'm suggesting and then we could discuss further as I don't believe the changes I'm suggesting would really be that inelegant.

MrToph commented 1 year ago

some of it has been implemented in #4