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

Allow funds to be sent directly to domains #1291

Draft
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

area
Copy link
Member

@area area commented Sep 13, 2024

This is work that I've done while working on #1285 as a prerequisite for the required functionality of domains to be able to manage and exchange tokens cross-chain. For that functionality to work, I believe this functionality (or something equivalent) is required; where such an exchange is happening cross-chain, and we can't directly interrogate balances before and after the exchange on the other chain, we need a more generic way to send tokens to a domain directly in order to do the bookkeeping correctly.

Because we can't send extra data with an ERC20 token transfer that's executed by other parties, I believe the only way to do this is have a system where each domain has a unique address that can have tokens swept from it in to the corresponding domain.

Fortunately, we can (re-)use CreateX for this. By using a contract deployment salt that's a function of the colony address and domain id, we can deploy a helper contract (DomainTokenReceiver) to a fixed address for each domain (even across chains) with simple functionality to allow the sweep to occur.

I've tried to make this as transparent as possible - the deployment, upgrading, and management of the receiving contract is intended to not require any explicit interaction from the user. The experience should be that the users send tokens to a domain-specific address (which they can get from getDomainTokenReceiverAddress on ColonyNetwork) and then claim them (via claimDomainFunds on Colony) without having to check whether the contract is already deployed and/or up-to-date.

Tagging this as ready-for-review because I want feedback, but leaving as draft because I don't want it merged yet. I think the biggest question I have hanging over this implementation is what the split between the Colony and ColonyNetwork should be for the implementation but open to feedback from any and all directions.

@area
Copy link
Member Author

area commented Sep 13, 2024

An open question here is what should happen in terms of the colony reward pot. I think I am of the opinion that whatever it is set to should be respected.

@arrenv
Copy link
Member

arrenv commented Sep 13, 2024

So, funds sent to the main colony address or specific domain addresses will also automatically be divided up and split amongst the respective teams set by the rewards pot? Regardless if the funds are only sent to a specific team?

@area
Copy link
Member Author

area commented Sep 13, 2024

split amongst the respective teams set by the rewards pot

This isn't really describing what the reward pot does. It is a single pot, and the only configuration available related to it is the fraction of incoming funds that gets put in to it, and those funds will eventually be divided among all people who have both reputation and native tokens.

It's possible this is functionality that should be removed, but until that decision is made, anything other than respecting that configuration means that it can be totally bypassed (as instead of sending funds to a colony directly, you can send funds directly to the top-level domain).

@arrenv
Copy link
Member

arrenv commented Sep 13, 2024

This isn't really describing what the reward pot does. It is a single pot, and the only configuration available related to it is the fraction of incoming funds that gets put in to it, and those funds will eventually be divided among all people who have both reputation and native tokens.

It's possible this is functionality that should be removed, but until that decision is made, anything other than respecting that configuration means that it can be totally bypassed (as instead of sending funds to a colony directly, you can send funds directly to the top-level domain).

Haha, yeah, just crisscrossed myself with the talk of teams.

I agree with you on both. Perhaps in needs to be rethought a little or removed, but until that time, if it is configured, it seems to make sense that it can't be bypassed.

It just does not seem intuitive that it would be the case when a colony is transferring funds to itself, but, I understand that this is how this functionality would essentially work.

@area area force-pushed the feat/fund-domains-directly branch 4 times, most recently from d698cfc to d5c36c3 Compare September 24, 2024 14:38
Copy link
Member

@kronosapiens kronosapiens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, will probably have some more comments on my next go-round but here are my first reactions.

contracts/colonyNetwork/ColonyNetworkDeployer.sol Outdated Show resolved Hide resolved
contracts/colonyNetwork/ColonyNetworkDeployer.sol Outdated Show resolved Hide resolved
contracts/colonyNetwork/ColonyNetworkDeployer.sol Outdated Show resolved Hide resolved
contracts/colony/ColonyFunding.sol Show resolved Hide resolved
@area area force-pushed the feat/fund-domains-directly branch 3 times, most recently from c978a65 to 569309e Compare September 26, 2024 17:05
@area
Copy link
Member Author

area commented Sep 30, 2024

isContract is now being duplicated and could be refactored out.

EDIT: Sort-of on hold until Arren and Jack have thought more about what compromise is acceptable in terms of someone looping payments to create reputation and take over a colony.

@kronosapiens
Copy link
Member

Could say that internal tokens cannot be sent directly to domains?

@area
Copy link
Member Author

area commented Oct 2, 2024

Could say that internal tokens cannot be sent directly to domains?

Essentially, yes, this is what I was going to implement (or, more strictly, when such tokens are claimed, they go to the root domain) but there are concerns it will render the functionality less useful than they would like.

@area area force-pushed the feat/fund-domains-directly branch from 569309e to 448f72c Compare October 2, 2024 16:29
@area area force-pushed the feat/fund-domains-directly branch 2 times, most recently from 4ac35aa to f888d3d Compare October 8, 2024 14:35
@area
Copy link
Member Author

area commented Oct 10, 2024

The solution that has been decided on is to let root approve a certain amount of reputation-earning tokens to enter a domain (like ERC20). Any above the approved amount are sent to root.

The intention is that the typical in-domain swap (through the UI, at least) that would result in a domain receiving reputation-earning tokens would be a motion that requires root that makes the approval for an appropriate amount and then does the swap.

Motions being approved at the same time should be supported (or at least not break in a surprising way).

@area area force-pushed the feat/fund-domains-directly branch 3 times, most recently from 689000d to 82593ee Compare October 14, 2024 11:12
@area area force-pushed the feat/fund-domains-directly branch from e511717 to e1b6828 Compare October 15, 2024 12:51
@area area force-pushed the feat/fund-domains-directly branch 2 times, most recently from bea8896 to 2ec9efe Compare October 21, 2024 15:38
contracts/colony/Colony.sol Outdated Show resolved Hide resolved
contracts/colony/ColonyFunding.sol Show resolved Hide resolved
contracts/colony/ColonyFunding.sol Outdated Show resolved Hide resolved
contracts/colony/ColonyFunding.sol Outdated Show resolved Hide resolved
contracts/colonyNetwork/ColonyNetworkDeployer.sol Outdated Show resolved Hide resolved
contracts/common/DomainTokenReceiver.sol Show resolved Hide resolved
contracts/common/DomainTokenReceiver.sol Outdated Show resolved Hide resolved
test/contracts-network/colony-funding.js Outdated Show resolved Hide resolved
Copy link
Member

@kronosapiens kronosapiens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

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.

3 participants