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

on-chain application statuses proposal #1

Merged
merged 5 commits into from Mar 21, 2023
Merged

Conversation

gravityblast
Copy link
Member

@gravityblast gravityblast commented Mar 15, 2023

This this PR we add a proposal to change our Round contract to store application statuses on-chain.

Details here:

https://github.com/allo-protocol/aips/pull/1/files


AIP 1 is reserved to describe the AIP repo in general when we have time to do it.

AIPS/aip-2.md Outdated Show resolved Hide resolved
@nfrgosselin
Copy link
Contributor

I'm supportive of these changes — thank you guys for authoring AIP #1 🎉

@0xKurt
Copy link
Contributor

0xKurt commented Mar 16, 2023

The proposed modification to the smart contract, which omits the project owner check, poses a significant risk to the integrity of the stored data - as already known.

As the applications that are stored within the contract can be created by anyone, the resulting maintained lists cannot be deemed trustworthy.

While verification of data authenticity may still be achievable on the frontend, the on-chain state itself would be unusable, which is somehow against the fundamental principle of smart contracts.

I suppose that it would fail to meet the standards required to pass an audit.

@vacekj
Copy link

vacekj commented Mar 16, 2023

What Kurt said is a big problem - I would be for addressing this before passing this proposal.

@boudra
Copy link

boudra commented Mar 17, 2023

@KurtMerbeth @vacekj my understanding from reading the proposal is that this is a non goal.

Correct me if I am wrong but I think this is already an issue with the current implementation, and the scope of this proposal is to bring those statuses on chain, which will already improve things, this verification could be a separate piece of work.

Or are you proposing to increase the scope of the proposal? if so, do we have a proposed solution?

@0xKurt
Copy link
Contributor

0xKurt commented Mar 17, 2023

@boudra The only way to solve it for now without going beyond the scope is having single-chain-applications only.

@gravityblast
Copy link
Member Author

thank you for your comments @KurtMerbeth @vacekj !

as @boudra said, this change doesn't add or change that possible problem, and it's also a non-goal specified in the AIP.
We already have the same logic, and if we want to change it I suggest to create a new AIP for that.

The proposed modification to the smart contract, which omits the project owner check, poses a significant risk to the integrity of the stored data - as already known.

The main problem in the current system is that we don't have any data on-chain, and it's only accessible from the graph, which doesn't reflect what happens on-chain. Instead, it allows only 1 application for a given project.

From a product perspective we want to allow projects to apply multiple times to have a better edit flow.

  • Project 1 applies with application 1
  • Application 1 is accepted
  • Project 1 re-applies with application 2
  • Application 1 is still accepted and possibly visible in the explorer so Project 1 doesn't lose visibility.
  • Round operator accepts application 2 and cancels application 1
  • The only valid application is now application 2

As the applications that are stored within the contract can be created by anyone, the resulting maintained lists cannot be deemed trustworthy.

There's a review process that the round operators wants to do.
By default all applications are pending, so they are only a signal from the project owners to the round operator.
The trust is in the round operators, they are the one with the final word on the applications to be accepted/rejected/cancelled.

While verification of data authenticity may still be achievable on the frontend, the on-chain state itself would be unusable, which is somehow against the fundamental principle of smart contracts.

I think it's usable if you follow the logic of the contract and the logic you want to have in your contract:

  • Pending applications are only signals from projects owners to apply to a round
  • You need to check the status of the application to check its validity.

On-chain data it's not true by default, I can send you a soulbound token or a bad attestation that you can't remove, it depends if it's valuable or not for the domain you are working on.

I suppose that it would fail to meet the standards required to pass an audit.

It depends on the meaning we give to the list of applications. If we send some value to any application then yes, it would be a big flaw.

But we have a list of pending applications by default, and only the round operator decides which one will be accepted. We could even remove the projectID and just accept applications, but we want it as a helper to link project and applications.

Apply to a round costs money/gas, and now that you cannot override an application there's no incentive to spam.

@kevinrolsen
Copy link

I'm supportive of solving these two problems separately (status and project ownership).

I feel that between the implementation of this AIP and a cross chain project ownership verification AIP we have at worst "dirty" application data, but as @gravityblast points out this would be reviewable at the app layer and this data would always be in a "pending" status on chain.

Users of the protocol would need to be aware that only "approved" applications should be trusted, and that trust comes from off-chain logic and human reviewers in the app layer.

I feel that this is a fair compromise to allow us to iterate on the protocol without increasing scope, and support adopting this AIP.

@vacekj
Copy link

vacekj commented Mar 17, 2023

Now I understand this much better, thank you for the explanation @gra. I agree that we should go forward with this AIP and solve the other problems in another one.

@gravityblast gravityblast merged commit 34c60a0 into main Mar 21, 2023
@gravityblast gravityblast deleted the on-chain-statuses branch March 23, 2023 14:27
@gravityblast
Copy link
Member Author

merged as implemented in allo-protocol/allo-contracts#5

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

Successfully merging this pull request may close these issues.

None yet

7 participants