-
Notifications
You must be signed in to change notification settings - Fork 12
Spacemesh api mock [WIP] #42
base: master
Are you sure you want to change the base?
Conversation
@avive @samparsky I will make the changes thanks for the review. |
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'm missing a test that just uses the mock I can run to validate the code and copy to my own test.
} | ||
|
||
const SpacemeshApiClientMock = <P extends IProps>(Component: React.ComponentType<P>) => { | ||
return class SpacemeshApiClient extends React.Component<P & IProps, IState> implements ISpacemeshApi { |
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.
Clean code and good documentation, thanks.
I came back here for a second pass and am missing the class-level comment :-(
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.
@daonb What do you mean by the class-level comment?
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.
Seems like I missed the line, sorry. I meant you should add in line 12 something like:
/**
* A class representing a mocked API endoint
*/
@@ -41,16 +41,17 @@ const SpacemeshApiClientMock = <P extends IProps>(Component: React.ComponentType | |||
/** | |||
* Broadcast a transaction to the Spacemesh network | |||
* @param tx raw transaction to broadcast | |||
* @param hash the transaction hash. |
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.
hmm... this changes the API so the mock now has a different interface. I prefer we don't break API compatibility and add a class property: nextResult
, give it an initial value ('0xe670ec...') and return its value wherever the mock return a result. This way we can keep this function's interface free of Mock-specific parameters and give the use the abilit to set the property to a different value if the test requires it.
* @returns transactions hash | ||
*/ | ||
public boradCastTransaction = (tx: ITransaction): Promise<MockData.IBroadCastTransactionResponse> => { | ||
public boradCastTransaction = (tx: ITransaction, txhash: string): Promise<MockData.IBroadCastTransactionResponse> => { |
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's a typo here: replace boradCastTransaction
with broadcastTransaction
.
This is just a draft.
I have some questions @avive @iamonuwa :
Is the general implementation pattern proper, and what you guys where thinking of ?
Should I add auto-fix to
TSlint
? And why are there lint rules that wont allow me to use a type forTransaction
but instead request ainterface
Could you provide some better mock data and structure for example how should
buySMCFromExchange
work exactly should it search for the cheapest price on a list of exchanges or should it let the user specify the exchange with some sort of ID?Closes #28