-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: added combobox, get documentstore, validate documentstore #38
Conversation
✔️ Deploy Preview for new-admin-opencerts ready! 🔨 Explore the source changes: 4ce9ee1 🔍 Inspect the deploy log: https://app.netlify.com/sites/new-admin-opencerts/deploys/611ca65fe168d900079e4b77 😎 Browse the preview: https://deploy-preview-38--new-admin-opencerts.netlify.app/ |
This pull request introduces 2 alerts when merging f2576a6 into 5395197 - view on LGTM.com new alerts:
|
|
||
export const isDocumentStore = async (documentStoreAddress: string): Promise<boolean> => { | ||
const documentStoreArray = await getDocumentStores(); | ||
documentStoreArray.filter((documentStore) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should only allow document stores we retrieved. What if we missed some?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you didn't address this ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I not using isDocumentStore to validate the contract anymore, thus removing the function. I using isValidContract() located in common.tsx. The function checks if the address is a contract using getCode that get blockTag block height, If the address is not a contract, the result return is 0x
Reference
https://docs.ethers.io/v5/single-page/#/v5/api/providers/provider/-%23-Provider-getCode
@@ -0,0 +1,39 @@ | |||
import { connectWallet, getWalletNetwork } from "./wallet"; | |||
|
|||
export const getDocumentStores = async (): Promise<Array<Record<string, unknown>>> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do you test? It doesn't look like it work at all for me.
My wallet: https://ropsten.etherscan.io/address/0xB26B4941941C51a4885E5B7D3A1B861E54405f90
All the document stores are less than 63 days old + I didn't create that many in the last 2 months
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i found 100 contract creation log for "0xB26B4941941C51a4885E5B7D3A1B861E54405f90" wallet
https://ropsten.etherscan.io/txs?a=0xB26B4941941C51a4885E5B7D3A1B861E54405f90&f=5&p=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found the issue. fixed. The logs topic is only capturing address "0xc84b0719a82626417c40f3168513dfabdb6a9079". i have change it to get logs based on the wallet address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I see some doc store, but I can't find https://ropsten.etherscan.io/address/0x8Fc57204c35fb9317D91285eF52D6b892EC08cD3
what about u ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0x8Fc57204c35fb9317D91285eF52D6b892EC08cD3 seems to be a very different from the current contract, it is using the older contract version.
Based on my checks, it is currently retrieving the contracts that are of the current version, this feature will cause an issue if there is an upgrade on the upcoming/future contracts refactoring. i believe it is not worth continuing the feature of getting a document store as there isn't a simpler way to retrieve the past document store.
If u are okay with it, I will remove the retrieval of the document store and combo box but keep the document store validation just by checking the hashes length. This MR is getting huge, it will be preferable if i could get this merged into master before I change it to the localStorage implementation.
}; | ||
|
||
return ( | ||
<Creatable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't use arrows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do u mean by can't use arrows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to navigate with arrows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. The issue is cause useEffect continuous looping recreating the dropdown. I have fixed it by running useEffect only once
@@ -41,6 +41,7 @@ | |||
"@types/node": "^15.12.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read the other comments but overall I don't think it's worth continuing on this. Doesn't look like a simple way t do what I suggested unless you want to dig further.
We can keep the part that verifies it's a valid address
Let me know what you think.
This pull request introduces 1 alert when merging c896eaa into 4e6761b - view on LGTM.com new alerts:
|
@@ -1,13 +1,15 @@ | |||
import React, { Dispatch, FunctionComponent, useState } from "react"; | |||
import { TextInput } from "./common/text-input"; | |||
declare let window: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- dont put ts definitions in ts code
- you completely delete all the provided typings :)
@@ -41,7 +41,7 @@ export const Header: FunctionComponent = () => { | |||
getWalletDetails(); | |||
} | |||
} | |||
}); | |||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
u should remove the listeners
getDocumentStoreList(); | ||
} | ||
} | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove listeners
Added combo box for document store input
Retrieve all the documents store created by the wallet
Validate if the document store belongs to the wallet
Closes #34 , Closes #36