Baseline Calender #480
Baseline Calender #480
Conversation
vaibhavgeek
requested review from
skarred14,
kthomas,
ognjenkurtic,
rfisch,
Therecanbeonlyone1969,
fosgate29,
Manik-Jain,
skosito and
Kasshern
as code owners
There was a problem hiding this comment.
Hey @vaibhavgeek i did one pass, and there are some comments, but i realized this is not ready for review, not sure if your intention was to go step by step, but here are some suggestions:
- please update readme to indicate what is done, what is missing
- if you are adding more changes, like frontend etc, please open new PR, and make this one self contained
- please leave TODOs instead of copying whole files from other repos (for example, battleship circom file here is copied as calendar circom file)
- since most of this code is actually using this repo https://github.com/amaurym/login-with-metamask-demo you will have to check licence and follow on rules of that licence, which i think it is just providing original licence and copyright claims if there are any, and also reference that repo
- your readme is just grant application readme, please change that to explain intention of calendar example, how it is example of baseline process, how to run it etc, because atm this is a crud backend app without baseline in it - again i realize this can be just first step, but it is not indicated anywhere what is the scope of PR
- please rename calender to calendar everywhere
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${accessToken}` | ||
| } | ||
| })).data; |
There was a problem hiding this comment.
possible error if axios call fails
There was a problem hiding this comment.
Updated the code.
| headers: { | ||
| Authorization: `Bearer ${accessToken}` | ||
| } | ||
| })).data.appointment; |
There was a problem hiding this comment.
possible error if axios call fails
There was a problem hiding this comment.
Updated the code .
| } | ||
| } | ||
|
|
||
| const coinbase = await web3.eth.getCoinbase(); |
There was a problem hiding this comment.
Do we need coinbase instance for this PR?
Can we not use ethereum instance to get the selected Address?
There was a problem hiding this comment.
This includes the case scenario if the person has coinbase wallet but not metamask, so it serves for both.
| setLoading(true); | ||
|
|
||
| // Look if user with current publicAddress is already present on backend | ||
| const users = (await axios.get<User[]>(`${process.env.REACT_APP_BACKEND_URL}/users?publicAddress=${publicAddress}`)).data; |
There was a problem hiding this comment.
possible error if axios call fails
There was a problem hiding this comment.
Updated the code.
| const {publicAddressSigned, signature} = await handleSignMessage({"publicAddressSigned": publicAddress, "nonce": users[0].nonce}); | ||
| const auth = await handleAuthenticate({"publicAddress": publicAddressSigned, "signature": signature}); | ||
| await onLoggedIn(auth) | ||
| onLoggedIn(auth); |
There was a problem hiding this comment.
why are we calling the same onLoggedIn twice?
There was a problem hiding this comment.
Removed one instance of login.
| const {publicAddressSigned, signature} = await handleSignMessage({"publicAddressSigned": publicAddress, "nonce": user.nonce}); | ||
| const auth = await handleAuthenticate({"publicAddress": publicAddressSigned, "signature": signature}); | ||
| await onLoggedIn(auth) | ||
| onLoggedIn(auth); |
There was a problem hiding this comment.
Removed one instance of login.
| headers: { | ||
| Authorization: `Bearer ${accessToken}` | ||
| } | ||
| })).data; |
There was a problem hiding this comment.
possible error if axios call fails
There was a problem hiding this comment.
Updated the code.
| headers: { | ||
| Authorization: `Bearer ${accessToken}` | ||
| } | ||
| })).data; |
There was a problem hiding this comment.
possible error if axios call fails
There was a problem hiding this comment.
Updated the code.
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${accessToken}` | ||
| } | ||
| })).data; |
There was a problem hiding this comment.
possible error if axios call fails
There was a problem hiding this comment.
Updated the code.
| const timesInitial = () => { | ||
| const timeReturn = []; | ||
| var someDate = new Date(selectedDate); | ||
| someDate.setHours(8, 0, 0); |
There was a problem hiding this comment.
please add a JSDoc as to what is happening here and at line 78
There was a problem hiding this comment.
It gets the Time availability API. I feel the code is self explanatory here.
| const booked = await getTimeAvailAPI(id); | ||
| booked.forEach((booking: any) => { | ||
| const tempTimes = [...times]; | ||
| var foundIndex = tempTimes.findIndex(x => parseInt((x.startTime.getTime()/1000).toString()) == (booked.timeStart/1000)); |
There was a problem hiding this comment.
sorry, I didn't understand the logic here.
Are we assuming that x.startTime andbooked.timeStart are going to be A * 1000 values? what if x.startTime is 1234567, and hence logic returns 1234.567`
Is that acceptable?
There was a problem hiding this comment.
Yes, the condition is just on equality. It's just that server responses were few ms delayed and hence there was an error if I just directly compared them.
There was a problem hiding this comment.
Updated the code.
| const username = user && user.username; | ||
|
|
||
| return ( | ||
| <div className="Profile"> |
There was a problem hiding this comment.
quite same content is in another file too. Can this be extracted out, and used in different files
There was a problem hiding this comment.
Just two files, would have made sense if there were more than 2 instances.
| username: string, | ||
| publicAddress: string, | ||
| nonce: string, | ||
| createdAt: string, |
There was a problem hiding this comment.
I think we don't need createdAt and updatedAt in front-end
There was a problem hiding this comment.
Updated the code.
| @@ -0,0 +1,26 @@ | |||
| /* eslint-disable @typescript-eslint/ban-ts-comment */ | |||
There was a problem hiding this comment.
This is an IEF, do we need this file?
There was a problem hiding this comment.
If someone else wants to test out the circuit independently without running the entire backend-frontend, this file helps a lot. I strongly recommend to keep it.
There was a problem hiding this comment.
Okay, but the purpose of this file is not clear. Can you add a JSDoc comment?
There was a problem hiding this comment.
Tests out the fixed merkle tree.
| ); | ||
|
|
||
| /* | ||
| // Changed by Jordi point |
There was a problem hiding this comment.
Not written manually
There was a problem hiding this comment.
okay, but the unused content can be deleted, right?
There was a problem hiding this comment.
I would rather keep it because the comments are also generated by the circuit compiler. It will get overwritten when the person runs the compiler themselves. The same is also seen in battleship example.
| ); | ||
|
|
||
| vk.beta2 = Pairing.G2Point( | ||
| [7510344623763166301235773457354776013635624944306764504873170219490212633507, |
There was a problem hiding this comment.
If somebody tries to extend this work, it will be better to add comments to signify what these numbers relate to.
There was a problem hiding this comment.
This is polynomialization of the circuit by circom code, not manually written code. This is generated on running
sh setup.sh and sh proof.sh files.
There was a problem hiding this comment.
approved from my side too, we will merge after comments from @Manik-Jain are addressed too
|
@vaibhavgeek Could you please push your latest changes? I see the comments, but not the updated code. |
vaibhavgeek
dismissed stale reviews from skosito and Therecanbeonlyone1969
via
f589970
|
@Manik-Jain I have made changes to the files requested in the frontend as well, thanks for reviewing the pull request in detail. |
vaibhavgeek
requested review from
Therecanbeonlyone1969,
skosito and
Manik-Jain
There was a problem hiding this comment.
Looks like your comments were addressed with the last commit @Manik-Jain
Description
This pull request completes Milestone 1 and Milestone 2 of (ethereum-oasis-op/baseline-blips#24) which involves creation of backend APIs and circom circuit.
https://github.com/eea-oasis/baseline/tree/feature-calendar-BLIP/examples/calendar
Baseline Calendar is an example project which involves a privacy centric way of scheduling appointments. More details can be found here
Related Issue
ethereum-oasis-op/baseline-grants#70
Motivation and Context
To create a fully working implementation of baseline that can be used as another example of the power baseline has.
How Has This Been Tested
All the API endpoints have been tested manually, the circuit has been tested with custom inputs.
Screenshots (if appropriate)
Types of changes
Checklist