Skip to content

Deliver OpenSquare offchain voting#297

Merged
alxs merged 3 commits intow3f:masterfrom
opensquare-network:master
Nov 18, 2021
Merged

Deliver OpenSquare offchain voting#297
alxs merged 3 commits intow3f:masterfrom
opensquare-network:master

Conversation

@wliyongfeng
Copy link
Copy Markdown
Contributor

@wliyongfeng wliyongfeng commented Oct 28, 2021

Milestone Delivery Checklist

Link to the application pull request: w3f/Grants-Program#544

Co-authored-by: Aleixo Sánchez <15819210+alxs@users.noreply.github.com>
@wliyongfeng wliyongfeng requested a review from alxs October 29, 2021 07:15
@alxs alxs self-assigned this Oct 29, 2021
@wliyongfeng
Copy link
Copy Markdown
Contributor Author

Hi @alxs , have to review our delivery? Looking forward to your feedback.

@alxs
Copy link
Copy Markdown
Contributor

alxs commented Nov 16, 2021

Hey @wliyongfeng, I hadn't had the chance yet, thanks for the ping.

Looks great all in all, both the UI and UX are very smooth. I'm not sure I understand the structure of the repo though, could you add a brief outline to the main readme?

Try to run the next package locally as per its readme I ran into

yarn run v1.22.17
$ node server.js
node:internal/url:552
  throw new ERR_INVALID_URL(input);
  ^

TypeError [ERR_INVALID_URL]: Invalid URL
    at new NodeError (node:internal/errors:371:5)
    at onParseError (node:internal/url:552:9)
    at new URL (node:internal/url:628:5)
    at Object.<anonymous> (/media/shared/Software/repos/eval/collaboration/packages/next/server.js:10:16)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12)
    at node:internal/main/run_main_module:17:47 {
  input: 'undefined',
  code: 'ERR_INVALID_URL'
}

And one of the unit tests fails since DECOO_API_SECRET_KEY is undefined which I think is pretty self-explanatory so from my side it's fine.

There's one user story I wasn't able to verify though, namely number 3 as it's not clear that one has voted on a proposal already, apart from it being somewhere among the votes. User story 4 isn't fully met either since it's not possible to pick a result strategy at the time of creation. Could you address these?

Many thanks.

@wliyongfeng
Copy link
Copy Markdown
Contributor Author

@alxs Will update the readme soon. The DECOO related configuration is for IPFS uploading.

namely number 3 as it's not clear that one has voted on a proposal already, apart from it being somewhere among the votes.

Sure, currently a user can only check his previous vote in the vote list. Indeed it's better with a component to show the user current voting.

User story 4 isn't fully met either since it's not possible to pick a result strategy at the time of creation. Could you address these?

We changed the implementation since currently there are only 2 strategies: balance-of and quadratic-balance-of, and we just provide all these 2 strategy voting result to avoid user choice at the creation time. Will add interfaces for user to choose strategies when 1. we implemented more 2. a space is configured maybe more than 3 or 4 strategies. User can still choose one strategy voting result in current implementation. Does it make sense?

ogtAmi6yqw

@alxs
Copy link
Copy Markdown
Contributor

alxs commented Nov 16, 2021

Sounds good!

Sure, it also works like this - I just felt it was a bit confusing as it's just not clear which of the two will be chosen, and it makes sense for the proposer to choose it when they define the proposal anyway. Feel free to implement it later.

@wliyongfeng
Copy link
Copy Markdown
Contributor Author

wliyongfeng commented Nov 18, 2021

Hi @alxs, we updated the readme and improved the 'my vote' view on the proposal detail page.

Generally the code is organized by 2 packages, node-api is in charge of chain node interaction while we maintain multiple api instances and fetch chain info with Promise.any, so we can fetch chain info when >= 1 node work well. The next package provide fronted pages, do SSR and provide apis.

'My vote' view works like the following image. It will always be at the top of the votes list, when the address is connected only of course. Please confirm it on the proposal detail page.
sFJ7jflELK

And the unit test is also fixed, unit test should mock the environment variable by itself , many thanks!

@alxs
Copy link
Copy Markdown
Contributor

alxs commented Nov 18, 2021

@wliyongfeng the readme provides everything I was looking for - many thanks for the changes! You may also want to add instructions on getting MongoDB up and running, which I still had to figure out.

With this, I'm happy to tell you that the milestone is a pass. You can find my evaluation notes here. I'll forward your invoice for internal processing - please allow up to 15 days until the payment is made.

@alxs alxs merged commit 4594fdf into w3f:master Nov 18, 2021
@RouvenP
Copy link
Copy Markdown

RouvenP commented Nov 26, 2021

hi @wliyongfeng we transferred the payment today.

@wliyongfeng
Copy link
Copy Markdown
Contributor Author

hi @wliyongfeng we transferred the payment today.

Confirmed, many thanks.

robcxyz pushed a commit to geometry-labs/Grant-Milestone-Delivery that referenced this pull request Dec 18, 2021
* Deliver OpenSquare offchain voting

* Correct file name

* Update deliveries/OpenSquare_offchain_voting-milestone_1.md

Co-authored-by: Aleixo Sánchez <15819210+alxs@users.noreply.github.com>

Co-authored-by: Aleixo Sánchez <15819210+alxs@users.noreply.github.com>
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.

3 participants