feat: walletconnect dependencies and component working #3
Conversation
andreabadesso
requested review from
alexruzenhack
and removed request for
pedroferreira1
* chore: working Dockerfile * chore: working now in Standalone mode * chore: some last fixes to be ready for production * fix: DynamoDB configuration
andreabadesso
requested review from
pedroferreira1
and removed request for
alexruzenhack
andreabadesso
requested review from
alexruzenhack and
pedroferreira1
and removed request for
pedroferreira1 and
alexruzenhack
| } catch (e) { | ||
| } |
There was a problem hiding this comment.
[question] It is ok to have a silent error here? Shouldn't it be interesting to have at least an error log?
| hathorRpc: IHathorRpc, | ||
| title: string, | ||
| description: string, | ||
| oracleType: string, | ||
| oracle: string, | ||
| timestamp: number, | ||
| token: string, |
There was a problem hiding this comment.
[question] There is any validation requirement here?
There was a problem hiding this comment.
No, but we're validating the input in the form
| throw new Error('No timestamp received in transaction'); | ||
| } | ||
|
|
||
| await createNanoContractTx(nanoContract, title, description, oracleType, oracle, timestamp, nanoContract.timestamp); |
There was a problem hiding this comment.
[question] This method can throw, right? What the user see when this method throws?
There was a problem hiding this comment.
Oops, this file shouldn't exist, removed
| timestamp: Date, | ||
| }[]>([]); | ||
| const [bet, setBet] = useState<null | { amount: number, bet: string }>(null); | ||
| const [error, setError] = useState<boolean>(false); |
There was a problem hiding this comment.
[question] You don't need to show any feedback message to the user?
There was a problem hiding this comment.
We definitely could improve the error handling here, but it was not a requirement for this first release
| }, [getFirstAddress, hathorRpc, connect, nanoContract ]); | ||
|
|
||
| const onConnect = useCallback((e: React.MouseEvent<HTMLElement>) => { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
question: Why to prevent the default behavior?
There was a problem hiding this comment.
The default behavior would be to submit the form, which is something we don't want
| useEffect(() => { | ||
| (async () => { | ||
| const client = getClient(); | ||
|
|
||
| await _subscribeToEvents(client); | ||
| await _checkPersistedState(client); | ||
| setClient(client); | ||
| })(); | ||
| }, [_subscribeToEvents, _checkPersistedState]); |
There was a problem hiding this comment.
question [blocking]: If the dependencies change we need to clean the previous subscriptions, right? If so, this cleaning is lacking right now.
| if (response.status !== 200) { | ||
| throw new Error('Error creating nano contract.'); | ||
| } |
There was a problem hiding this comment.
question: Isn't it interesting to have a log here to inform the response status at least?
| interface Tx { | ||
| hash: string; | ||
| nonce: string; | ||
| timestamp: number; | ||
| version: number; | ||
| weight: number; | ||
| signal_bits: number; | ||
| parents: string[]; | ||
| inputs: any[]; | ||
| outputs: any[]; | ||
| tokens: any[]; | ||
| nc_id: string; | ||
| nc_blueprint_id: string; | ||
| nc_method: string; | ||
| nc_args: string; | ||
| nc_pubkey: string; | ||
| raw: string; | ||
| } | ||
|
|
||
| interface Meta { | ||
| hash: string; | ||
| spent_outputs: any[]; | ||
| received_by: any[]; | ||
| children: string[]; | ||
| conflict_with: any[]; | ||
| voided_by: any[]; | ||
| twins: any[]; | ||
| accumulated_weight: number; | ||
| score: number; | ||
| height: number; | ||
| min_height: number; | ||
| feature_activation_bit_counts: any | null; | ||
| first_block: string; | ||
| validation: string; | ||
| nc_block_root_id: any | null; | ||
| first_block_height: number; | ||
| } | ||
|
|
||
| interface ResponseObject { | ||
| success: boolean; | ||
| tx: Tx; | ||
| meta: Meta; | ||
| spent_outputs: any; | ||
| } |
There was a problem hiding this comment.
suggestion [not-blocking]: Extract it to a type file. The type file can be for the whole context of /lib/api.
| export interface NanoContract { | ||
| id: string; | ||
| timestamp: number; | ||
| title: string; | ||
| oracleType: string; | ||
| oracle: string; | ||
| description?: string; | ||
| createdAt: number; | ||
| } | ||
|
|
||
| export interface DynamoNanoContract { | ||
| id: AttributeValue; | ||
| timestamp: AttributeValue; | ||
| title: AttributeValue; | ||
| oracleType: AttributeValue; | ||
| oracle: AttributeValue; | ||
| description?: AttributeValue; | ||
| createdAt: AttributeValue; | ||
| } |
There was a problem hiding this comment.
suggestion [non-blocking]: Maybe it would be good to extract it to a type file as well. But it depends if we can use the interfaces without use the methods.
There was a problem hiding this comment.
question: why do we need this code?
There was a problem hiding this comment.
This is loaded in the create nano contract success screen to represent the code of the created nano contract
Acceptance Criteria