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

Merge features exclude content #490

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

Merge features exclude content #490

wants to merge 100 commits into from

Conversation

daodesigner
Copy link
Collaborator

No description provided.

samajammin and others added 30 commits December 14, 2021 11:02
Add notifications of recipient cap
* fetch maci factory data and save it in the store

* Refactor last maxRecipients getter

Co-authored-by: Sam Richards <[email protected]>
@yuetloo yuetloo self-assigned this Jul 4, 2022
@yuetloo yuetloo marked this pull request as ready for review July 4, 2022 21:05
@@ -177,16 +177,11 @@ contract OptimisticRecipientRegistry is Ownable, BaseRecipientRegistry {
*/
function executeRequest(bytes32 _recipientId)
external
onlyOwner
returns (bool)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@auryn-macmillan , do we want this change in the develop branch? This looks like ethStaker specific change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@auryn-macmillan , I removed this change anyways as it's failing unit tests. If you want this change, I can put it back and fix the test cases instead.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this was an ETHStaker specific change.

We could perhaps add this back in as a separate registry type in future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I added EthStaker's contract as PermissionedRecipientRegistry for now. But, the UI will still treat it as "optimistic".

netlify.toml Show resolved Hide resolved
component: RoundList,
meta: {
title: 'Rounds',
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

@auryn-macmillan I just realized the reason the rounds endpoint was removed is related to #448 and I'm still assessing the impact of the issue. So far i've found out that regardless of the round selected, the app always shows current round information because of the reset logic in the app.vue. Maybe we should even remove the /round/:id route as it has the same issue

Copy link
Member

Choose a reason for hiding this comment

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

Removing the /round/:id route would mean that users cannot view previous rounds, correct?

In an ideal world, I would prefer for the app to be able to display previous rounds. However, in practice this has turned out to be quite difficult, since previous rounds have often used different deployments of the contracts, earlier versions of the contracts, different configurations of contracts (recipient registries, etc), and/or been deployed to different networks. All of which is a huge pain to track in the context of one app.

Perhaps a cleaner solution is to treat each round as independent app and simply move previous rounds to a subdomain, like round1.clr.fund, when we begin a new round?

But, if we're confident that most people will not change networks and versions as often as we have (probably a reasonable assumptions), then it would be ideal for the app to support multiple rounds.

Copy link
Collaborator

@yuetloo yuetloo Jul 7, 2022

Choose a reason for hiding this comment

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

Correct, removing the /round/:id route will prevent users from viewing previous rounds.

Yeah, not sure how well the app supports cross-chain round viewing. It does support loading external previous rounds by setting the ipfs hash in VUE_APP_EXTRA_ROUNDS.

See the below screenshot, for external rounds, they have a little arrow beside the round. Clicking the round will open the ipfs url for the app hosting the round. So, this is similar to your solution, host previous rounds in a subdomain.

I would also like to get your opinion on whether the round information panel (the left panel) should always display the current round information or the selected round information depending on which page you're on?

/rounds:
Screen Shot 2022-07-07 at 10 13 27 AM

/projects
Screen Shot 2022-07-07 at 10 14 59 AM

/round/:id
Screen Shot 2022-07-07 at 10 14 45 AM

Copy link
Member

Choose a reason for hiding this comment

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

See the below screenshot, for external rounds, they have a little arrow beside the round. Clicking the round will open the ipfs url for the app hosting the round. So, this is similar to your solution, host previous rounds in a subdomain.

Yeah, that's a good point. I think it's reasonable to just leave it as is for now.

I would also like to get your opinion on whether the round information panel (the left panel) should always display the current round information or the selected round information depending on which page you're on?

My preference is for it to display the information on the selected round.

import { Project } from './projects'
import sdk from '@/graphql/sdk'

export const DEFAULT_CONTRIBUTION_AMOUNT = 5
export const MAX_CONTRIBUTION_AMOUNT = 10000 // See FundingRound.sol

// The batch of maximum size will burn 9100000 gas at 700000 gas per message
export const MAX_CART_SIZE = 13
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we make this parameter configurable and throw if it's not configured? This is to make sure we tested the max size before deploying to production.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@yuetloo yuetloo marked this pull request as draft July 7, 2022 20:05
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.

6 participants