zerolawtech / zap-tech Goto Github PK

The Issue

ZAP's current governance functionality is very limited:

1. The range of functionality that can be voted on is very limited. It can be expanded by combining it with the MultiSig permission scheme for sub-authorities, but it's a hacky approach that bends one thing to do another.
2. The result of a vote is a passive giving-of-permission, which means that votes can only allow access to already-existing functionality within ZAP.

Goals

1. Expand voting functionality in Governance.sol
   • time limited votes
   • ability to cast a no vote
   • formalized system for how proposals are submitted and who can do so
2. Build an equivalent to the Aragon agent:
   • use .call to forward abritrary calldata, allowing for active actions in the case of a passing proposal
   • integrate this within MultiSig authority and permissioning to allow truly decentralized ownership of contracts

• allow payouts in ERC20s
• incorporate MultiCheckpoint to allow complex payouts across multiple tokens
• recurring payments
• compatibility with NFToken
• test cases
• documentation

Certain smart contracts or components should renamed for consistency with whitepaper and maximum generality for all organizations/use cases. Here is my (WIP) proposal for renaming; I will update this from time to time as I work on the whitepaper:

• IssuingEntity --> OrgCode
• KYCBase --> IDVerifierBase
  • KYCIssuer --> OrgIDVerifier
  • KYCRegistrar --> RegistarIDVerifier
• TokenBase --> OrgShare
  • NFToken --> CertShare
  • SecurityToken --> BookShare
• Investor --> Member

Before this repo goes public, this should all be in place:

• contribution guide (standards and best practices, code style)
• issue templates (bug, feature request)
• PR template
• redo the readme.MD

This should have been done a loooong time ago.

Using of a fixed length array for Balance.ranges, as implemented in https://github.com/iamdefinitelyahuman/nftoken, is more gas efficient and should be implemented in this repo as well.

Per offline discussions, a poison pill could make for an interesting module that could be very useful in some DAO structures. How exactly to best implement this still needs to be explored.

cc @lex-node

Right of first refusal, right of second refusal, tag-along-drag-along... all of this could be enforced on-chain via a module. Opening this PR for discussion on how best implement.

cc @lex-node

• allow payment of options in ERC20s
• compatibility with NFToken (currently only works with SecurityToken)
• more tests
• documentation

Blacklists will work similar to the existing KYC whitelists, except any address listed on it is unable to transfer tokens. This could require some changes to IssuingEntity, and will take some thought on how conflicts between whitelists and blacklists work.

How this is implemented will be very dependent upon #9.

• explicit support for NFToken
• more tests

The Issue

Regardless of which address tokens are minted to, the Transfer and TransferRange events that fire will show them as being minted to msg.sender:

https://github.com/iamdefinitelyahuman/ZAP-Tech/blob/62f8434e9033a930f61e3ddb968a5ecd227ef535/contracts/NFToken.sol#L373-L374

How to Fix

Change msg.sender to _owner in the above lines, and maybe add some unit tests to check the correctness of the events being fired.

The Issue

In NFToken.transferFrom, _auth is set but not properly passed to _transfer:

https://github.com/iamdefinitelyahuman/ZAP-Tech/blob/62f8434e9033a930f61e3ddb968a5ecd227ef535/contracts/NFToken.sol#L565-L571

When the issuer uses transferFrom, this results in improper permission checks as well as incorrect modification of the allowed mapping.

How to Fix

• Correctly pass _auth to _transfer
• Write additional tests that check for correct behavior when transferFrom is called by the issuer

In a situation where it's desirable to have a token trade as freely as possible, there could be a use case for an identity registry that works via on-chain self attestation. On the tech side this isn't overly complex - the legal layer is where this could get challenging.

Implementation ideas (subject to change):

• If tokens are sent to an unknown address, they are frozen until the owner of the address self-attests.
• If the attestation does not occur within a set time period, the tokens are returned to the sender.
• Attestation will consist of broadcasting the hash of some form of legal document which proves the owner of the address has verified "i am from here, i am accreditted, i'm not on any black lists, etc etc etc"
• This can also be a way to deliver specific t&c for the token/org

cc @lex-node , we discussed this at length but it was a while ago, we should probably re-explore it to make sure the idea is sane before we begin implementing it

Currently all the contracts are marked as >=0.4.24 <0.5.0 - we should remove this ambiguity and declare them all as using version 0.4.25.

The Issues

• The tests use many fixtures that inherit one another - it's a very powerful setup but not easy to understand at first glance.
• conftest.py files are sometimes 3 folders deep, and individual tests declare their own fixtures
• Objects such as accounts are being imported that could instead be retrieved as fixtures
• Test file organization could be improved

How to Fix

• Add verbose comments within tests/conftest.py explaining what each fixture does
• Reorganize fixtures - avoid declaring them within a test file, keep conftest.py files at most 1 subfolder beyond tests
• handle imports as fixtures where possible
• produce more subfolder README.md files to make tests easier to navigate

The ultimate goal is to make the test suite understandable even to someone who has never seen ZAP before, so it can be linked to from the Brownie docs as an example of what is possible.

Currently the investor count information is deeply embedded within IssuingEntity. Even if a token should trade freely, it is impossible to stop tracking investor counts. In such a case this creates a massive gas overhead that is un-necessary.

Investor limit logic should be moved to a separate contract and treated as more of a "first class module" in the way the Governance currently works. This will be a big step away from IssuingEntity being the center hub of any deployment and toward each component interacting more directly.

This will be a big job and likely the intended spec for it will evolve during the process of completion.

It should be possible to set up a ZAP org that invests in other ZAP orgs - a sort of investment fund:

• ownership can be handled via a type of Custodian contract
• identify checks against individual org KYC registries can be performed whenever fund ownership changes
• members in the fund can use the (as yet undeveloped, #14) governance functionality to determine how it is run, what it invests in, when and how profits are distributed, etc

There are significant legal considerations in such a setup - @lex-node you will have to provide a lot of sanity checks in the review process on this one :)

Every core contract within ZAP should have an interface within contracts/interfaces, and where possible contracts should import these interfaces instead of importing contract directly. This will improve readability, help enforce the expected ABI, and ease third party integrations in the future.

draft a "ZeroLaw Manfiesto" or similar document setting forth core principles of how to pair law & tech for effective implementations

Istanbul's changes to gas costs make transfer less safe - possibly no changes need to happen, but this at least needs some deep examination.

Further reading - https://diligence.consensys.net/posts/2019/09/stop-using-soliditys-transfer-now/

Once #13 is completed we should add some basic CI checks:

• run tests
• linting
• build docs