allo-protocol/allo-v2

Core Allo V2 Contracts

allo-v2's Issues

Repo Cleanup

Update contracts to use address as recipient ID

We decided that an address will be the core recipientId that persists throughout the Allo contracts.

Changes we need to make include (please add other tasks as needed):

• Change applyToRound() to addRecipient() respectively.
• Revise DonationVoting strategy to account for this. ** not sure yet what branch to work from for this yet. We may want to merge the add-strategy branch first to work from a clean main.

Unused custom error in `DirectGrantsSimpleStrategy`

This error is declared but never used in the contract:

allo-v2/contracts/strategies/direct-grants-simple/DirectGrantsSimpleStrategy.sol

Line 48 in 3bc2f0e

error INVALID_RECIPIENT();

Create Allo.sol

per #11

• let's get to a solid V1 that is as simple as possible and accomplishes everything we need, primarily to interact with the strategies (we'll worry about view functions in a bit).

Merging Strategies

We just finished out call about merging strategies into one, and then building out a library of components instead.

This has the negatives of:

• not allowing easy mix and match cloning on chain
• requires developers to think a little more
• gives a vibe that Allo protocol is doing less

But it has the positives of:

• allowing us to modularly break down the logic more clearly
• better serve developers to help them mix and match for their own use cases (a la OZ)
• simplify the on chain logic to not require communication between strategies
• not have to worry about the interfaces between strategies

If we do this, I can imagine the reorganized repo would look something like this:

core/
    libraries/
        Metadata.sol
    Allo.sol
    Aqueduct.sol
    Registry.sol

components/
    IStrategy.sol
    BaseStrategy.sol (abstract contract, inherits from IStrategy)
    recipients/
        BaseRecipientsComponent.sol
        common/
            AllocationGating.sol
        ERC721AllocationGating.sol
        TokenBalanceAllocationGating.sol
        ...
    voters/
        BaseVotersComponent.sol
        common/
        ...
    allocation/
        BaseAllocationComponent.sol
        common/
        OffchainCalculations.sol
        ...
    distribution/
        BaseDistributionComponent.sol
        common/
        Splitter.sol
        ...

Then when a developer wants to create a strategy, they could do something like the below, and just override any functions they need (like using OpenZeppelin):

contract MyStrategy is BaseStrategy, ERC721AllocationGating, MyVotingComponent, OffchainCalculations, Splitter {}

Down the road, we can create something like https://wizard.openzeppelin.com/ to deploy mix and matches strategies automatically, or even put the components on chain.

Write tests for QV Simple

Donation voting with Passport

Strategy hitlist
Allocation module overview
Original strategy brainstorm

Registry.sol

Update donation voting spec

Spec: #65

This is part of the work to create our initial batch of allocation strategies. This spec is for the strategy that will replace the current Quadratic Funding implementation in Grants Stack.

Strategy hitlist
Allocation module overview
Original strategy brainstorm

Write tests for RFP Simple

rapped voting 🎤

Write alt registry contract

Use spec from #41

Write tests for QV NFT tiered

Write tests for Base Strategy

Create Linear QF allocation strategy

per #11

Note: As we create strategies, we'll put them in the appropriate folders and inherit from "../../interfaces/I{Type}Strategysol". We shouldn't need to create any other files to complete this. Let's see if we can hold ourselves to that.

Write tests for hackathon QV

Write tests for Allo.sol

Update simple rfp strategy

Spec: #67

This is part of the work to create our initial batch of allocation strategies. This spec is to update the simple RFP strategy here.

Strategy hitlist
Allocation module overview
Original strategy brainstorm

Write Simple QV strategy

Spec: #69

Strategy hitlist
Allocation module overview
Original strategy brainstorm

Write no application contract

Spec here: https://github.com/allo-protocol/allo-v2/tree/main/contracts/strategies/allocation/no-application

Write tests for RFP Committee

Write tests for Donation voting

Allocation Strategy 1 : Offchain QF

Update donation voting contract to send funds to donors

Currently the donation voting contract holds the donation funds — those funds should be sent directly to the recipients instead of being held temporarily.

WHOA BUDDY — HAVE ZACH REVIEW BEFORE STARTING THE BELOW TICKETS

Reorganize file structure

Per #11, we want to reorganize the contracts/ folder

The structure should be:

• interfaces/
  • IAllocationStrategy.sol
  • IDistributionStrategy.sol
• core/
  • libraries/Metadata.sol
  • Allo.sol
  • Registry.sol
  • Aqueduct.sol (note the slight spelling fix)
• strategies/
  • allocation/
  • distribution/

Note: this removes the utils/ folder (which just duplicates the logic in core/libraries/Metadata.sol).

Write RFP Committee strategy

Spec: #68

Strategy hitlist
Allocation module overview
Original strategy brainstorm

Update Registry.sol

per #11, point (3)

• update Registry.sol to align with the interface

Reminders:

• No linked lists
• Solmate Roles (with each identity having two types of roles, owner and member)
• IdentityID is just an incrementer
• attestationAddress is address(uint160(uint(keccak256(identityId, name))))

Incorrect `metadataRequired` initialization in `DirectGrantsSimpleStrategy`

allo-v2/contracts/strategies/direct-grants-simple/DirectGrantsSimpleStrategy.sol

Line 109 in 3bc2f0e

_metadataRequired = _metadataRequired;

It should have been:

metadataRequired = _metadataRequired;

Rename identity -> profile

Consolidate Registry.sol

per #11

• merge concepts and comments from IRegistry.sol into Registry.sol
• Remove IRegistry.sol

Update direct grants strategy

Spec: #66

This is part of the work to create our initial batch of allocation strategies.
Strategy hitlist
Allocation module overview
Original strategy brainstorm

Deploy to testnets

Deploy following contracts:

• Allo core
• Registry
• Donation voting

Deploy to following chains:

• Optimism Goerli
• PGN Testnet
• Sepolia
• Goerli

Allo.sol

Write Tiered NFT QV strategy

Spec: #71

Strategy hitlist
Allocation module overview
Original strategy brainstorm

Write tests for Registry.sol

Consolidate Allo.sol

per #11

• merge concepts and relevant concepts from IAllo.sol into Allo.sol
• remove IAllo.sol

rename allowedStrategies -> cloneableStrategies

Bootnode Direct Grants

This spec is for the strategy that will update Bootnode's direct grants implementation for Allo v2.

Strategy hitlist
Allocation module overview
Original strategy brainstorm

Enhancement ideas

From @KurtMerbeth

I would like to share my thoughts after walking through the strategy miro.
Maybe I can have your thoughts on it.

1. I would like to propose the addition of check functions for the functions in the core contract to enhance safety and usability:

   Examples:
   claim(uint _poolId, bytes memory _data) external;
   Proposed Check Function: isClaimable(uint _poolId);

   applyToPool(uint _poolId, bytes memory _data) external
   Proposed Check Function: canApply(uint _poolId, bytes memory _data)

   etc..

   These check functions will enable frontends, users and other contracts to verify if executing the corresponding operations is possible or not, based on predefined conditions or requirements.

2. I would like discuss the idea of introducing an execute function that would enable calling custom functions within Strategies through the Core contract. This feature aims to streamline pool handling and provide a unified approach for interacting with both standard and custom functions within Strategies.

   Pros:

   • Simplified Pool Handling: With the execute function, users and developers can interact with Strategies seamlessly through the Core contract, regardless of whether the functions are standard or custom. This streamlines pool handling and reduces the need to switch between different contracts, enhancing usability and developer experience.

   • Centralized Transaction Flow: By consolidating the function calls within the Core contract, the transaction flow becomes more centralized and easier to follow. This can improve the clarity and maintainability of the codebase.

   Cons:

   • Potential Security Risks: Introducing an execute function that allows arbitrary function calls to custom Strategy functions carries inherent security risks. Careful consideration must be given to access control and validation mechanisms to prevent unauthorized or malicious function executions.

   • Increased Complexity: Implementing the execute function adds complexity to the Core contract and may require additional validation and error handling logic. This can increase the development and testing effort required to ensure the robustness and reliability of the system.

3. Currently, the Core contract has a function called claim, but as we introduce distribution strategies that handle fund distribution to users, the name claim seems in some cases a bit misleading since claim is usually used as an action executed by an user.

   To address this, I suggest adding an adtional function called distribute to the Core contract. This function will be responsible for initiating the distribution of funds to users by the pool owner.

   Alternatively, we could think about introducing a new name which represents both, claim and distribute. I don't have any suggestions right now, but I was thinking about a more generic name like handleDistribution.

---

from @thelostone-mc

I would like to propose the addition of check functions for the functions in the core contract to enhance safety and usability:

I do like having these for enhancing safety check

Adding execute function.
I do see the benefit in flows like milestone based payout where :

• project owner submit milestone
• this function would exisist on the allocation strategy and in our current interface , the project owner would be forced to interact with the allocation contract instead of round
• Having the execute solves this but this feels quite scary. we could make it safer by saying no re-enterancy and maybe not payable.
  I'm split on this

rename claim as handleDistribution

In favour of this

---

from @KurtMerbeth

Having the execute solves this but this feels quite scary. we could make it safer by saying no re-enterancy and maybe not payable. I'm split on this

I feel the same. I wanted to encourage some thought on this matter.
Perhaps there is a viable way to implement something like this while ensuring safety. On the other hand, it's possible that the potential risks outweigh the benefits. Do we even consider it a problem worth addressing?

The idea of having one entry point for everything came up while I was going through the diagrams.

---

from @thelostone-mc

Would recommend having a updatePool on the interface cause we would def want to update the metadata
https://github.com/zobront/allo/blob/main/interfaces/IAllo.sol#L18

Also not relevant to the interface but we might want to add OZ multi-call to allow folks to allocate to multiple pools and stuff

---

from @codenamejason

I think multi-call here is a great idea and good call out @thelostone-mc as well as adding the updatePool() to the interface.

Do we need to return any data on an update or just emit an event?

function updatePool(bytes memory _data) external payable returns (bytes memory);

Create Distribute from a Mapping strategy

per #11

Note: As we create strategies, we'll put them in the appropriate folders and inherit from "../../interfaces/I{Type}Strategysol". We shouldn't need to create any other files to complete this. Let's see if we can hold ourselves to that.

Make project identity able to receive funds?

I know we keep having this discussion, but wanted to open it up again (for the fourth time?)

I think we may need to make it so that identities in the project registry are able to receive and hold funds (by deploying a lightweight proxy contract that people can only interact with through the Registry, similar to how Splits works). Reason being: I think it's just too natural to distribute payments to the anchor address

allo-v2/contracts/strategies/NFTVoteWinnerTakeAll.sol

Line 142 in dd1b6b1

_transferAmount(tokenToDistribute, currentWinner, amountToDistribute);

Write Hackathon QV strategy

Spec: #73

Strategy hitlist
Allocation module overview
Original strategy brainstorm

Skirting Fees

from @zobront
Starting a thread for us to discuss the fee skirting issue. The issue is that, since projects choose their own strategies, we can't have Allo protocol fees taken on the strategy, or users can easily skirt it. It needs to live on the core Allo.sol contract, but that poses challenges because when fees are taken may not be consistent across projects.

CURRENT WORKING SOLUTION

1. When a pool operator calls createPool(), they can add tokens and a fee is taken directly at that point. The total balance with which the pool has been funded is stored in the Allo.sol contract.

2. At any point, the operator can all fundPool() and add additional funds, and fees are taken at that point. The total balance with which the pool has been funded is updated in the Allo.sol contract.

3. When users call allocate() in a strategy that requires them to send additional funds (ie Quantitative Funding), that can be handled in two ways:
   a) If the funds go directly to the recipient, our prebuilt strategies take the fee directly at that point. If they deploy their own strategy, they could skirt this fee.
   b) If the funds go into the distribution strategy for later distribution, a function is called on Allo.sol that updates the total funding amount for the pool and takes the fee.

4. In order to trigger payouts, we call finalize() on Allo.sol. This checks the balance of the distribution strategy and, if it is greater than the stored amount of what it should be, takes any excess balance and transfers it to the Allo treasury.

ISSUES

1. An allocation strategy can be written that sends allocations straight to the recipient and pays no fees.

2. A distribution strategy can be written that skips over the finalize() function. We could add a flag to the main contract that is is now claimable once finalize() is called (which would stop claimers from using the core contract's claim() function if their distribution strategy skirts this step) but even that isn't perfect, because it doesn't account for distributions with ongoing phases.

---

from @KurtMerbeth

Additional Question, since we will have Pools with and without distribution strategies:
Are we taking only fees from the distribution, or also from allocations? @nfrgosselin

---

from @thelostone-mc

How would we ensure that the funds aren't sent directly to the DistributionStrategy and bypass the fundPool function ?

---

from @codenamejason

Can we do this by only allowing the Allocation Strategy to know the Distribution Strategy address or interact with the Distribution Strategy as the only owner? @thelostone-mc

---

from @nfrgosselin

Additional Question, since we will have Pools with and without distribution strategies:

Can you say more @KurtMerbeth? What types of pools wouldn't have distribution strategies?

Are we taking only fees from the distribution, or also from allocations?

My thinking was that the protocol would only take fees from the core pool (i.e. the distribution). Allocations could become separate income streams for strategy developers if they're creating novel strategies. So Gitcoin could deploy QF strategies that we technically take fees from, but for pools using those strategies we would actually be making fees on the distribution and the allocation. Another scenario are pools using externally-developed strategies, where the protocol would collect fees on the distribution and the strategy develop would collect fees on the allocation.

Test strategies sandbox with Optimistic Goerli

Goal is to confirm that strategies sandbox works with our deployment on OG

Write Governance QV strategy

Spec: #72

Strategy hitlist
Allocation module overview
Original strategy brainstorm

Action Plan

Thanks for the call this morning guys, and sorry I have been not as on top of it the past couple days.

I can feel things starting to sprawl a little bit, so want to make sure our actions for the rest of this week and really precise and focusing, rather than expansive.

Here is what I think needs to happen:

1) GET RID OF MOST INTERFACES

We've lost a single source of truth on a lot of this stuff. We can split sections out later, but for now, please remove IAllo.sol and IRegistry.sol and merge all the concepts and relevant comments into Allo.sol and Registry.sol. Then we'll be in a place to work to make those files perfect.

(The strategies can keep their interfaces because that's the only possible single source of truth, since they will all be different. I would recommend we delete the BaseAllocation.sol and BaseDistribution.sol and simply inherit straight from the interface in each strategy we create.)

2) REORGANIZE FILES

This is small but will help. Let's adjust the file structure to be:

• interfaces/
  • IAllocationStrategy.sol
  • IDistributionStrategy.sol
• core/
  • libraries/Metadata.sol
  • Allo.sol
  • Registry.sol
  • Aqueduct.sol (note the slight spelling fix)
• strategies/
  • allocation/
  • distribution/

Note that this removes the utils/ folder (which just duplicates the logic in core/libraries/Metadata.sol).

As we create strategies, we'll put them in the appropriate folders and inherit from "../../interfaces/I{Type}Strategysol". We shouldn't need to create any other files to complete this. Let's see if we can hold ourselves to that.

Let's try to push this pretty quickly too, so we're operating from a clean base. This may take some time though, and it'll require consolidating interfaces and contracts that may be different, so don't rush it!

3) MAKE MAJOR REGISTRY.SOL FIXES

The current Registry.sol seems to be based on the old one, and doesn't align with the interface. I think this needs to be right before we start building complete flows.

I would recommend creating this contract to the best of our ability first. It's relatively self contained and we already know the logic.

Reminders:

• No linked lists
• Solmate Roles (with each identity having two types of roles, owner and member)
• IdentityID is just an incrementer
• attestationAddress is address(uint160(uint(keccak256(identityId, name))))

4) CREATE ALLO.SOL

I think the logic on here will actually be relatively simple, since so much work is being pushed to strategies. So let's get to a solid V1 that is as simple as possible and accomplishes everything we need, primarily to interact with the strategies (we'll worry about view functions in a bit).

5) CREATE ONE STRATEGY PAIR

Pick the easiest possible strategy pair and code them up so the whole thing works.

I would recommend doing Linear QF for allocation and "distribute from a mapping" for distribution, but whatever you guys want.

6) VIEW FUNCTIONS

Think about if there are any view functions we know we need to add, both to the strategy interfaces and to core. Today, we spoke about amountClaimable(), isClaimable() and canApply().

Also consider whether there are additional states besides None, Pending, Approved, and Rejected that should be on the base status enum that projects need to implement.

---

Let's focus on those things first. There are other features worth exploring (like multicall) but I think we will be better served to consider those peripheral things, as well as building out the roster of strategies and making sure they all work, once the core piece is built.

I recognize that everything from STEP 4 on will rely on me figuring out the "finalize question" and fee skirting, so I will focus on that to make sure it's unblocked by the time you get there.

---

How does this plan sound to everyone? If good, I would say the Allo team can choose how to divide things up to tackle STEPS 1, 2, 3 (preferably in order and not in parallel, as there's a lot of restructuring that will make merging PRs messy) while I work on the "finalize stuff" over the next 2-3 days, and then we should be in a good place to bring all that together and start getting the rest of the system working.

Please include me as a review on these PRs too, so I can stay in the loop on new work being pushed.

Thanks!

can we move pendingOwner into a separate mapping?

          can we move pendingOwner into a separate mapping?

just because it's not an important identity information and there is no need to pass this information when you quey an identity

Originally posted by @KurtMerbeth in #24 (comment)

Write tests for Direct Grants simple

Distribution Strategy 1 : Splitter Distribution