-
Notifications
You must be signed in to change notification settings - Fork 3
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
Integrate with Kauri API :: GetRawTransactions #46
Conversation
…es :: .editorconfig
constructor(private http: HttpClient) {} | ||
|
||
getRawTransactions(addresses: 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.
we should always type the arrays - so string[]
avoid using any
where ever possible
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.
@red010b37 updated 🌮
styleUrls: ['./api-sample.component.css'] | ||
}) | ||
export class ApiSampleComponent { | ||
addresses = [] |
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.
we should always strongly type the arrays - so addresses = string[]
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.
Press the wroing btn
@red010b37 can you test this branch and let me know if you have any further suggestions? I made some additional updates last night |
@ro-savage Yeah it's setup to only accept wallet addresses, not txids. So in Postman, the request body would look like this:
And when testing from this branch, you would select NAV from the dropdown and then enter 2 NAV wallet addresses in the inputs. The logic in this PR will build out the proper request body (above) and POST it to the Kauri API running locally. The response will then contain all of the raw transaction data for each address. |
|
||
onGetTransactions(currency: string, addresses: string[]) { | ||
|
||
const apiModel: ApiModel = {} as ApiModel; |
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 thought the as
syntax was for overwriting the intefered type.
E.g. Type Assertions
However, it seems to be in a lot of places.
Would just const apiModel = {} as ApiModel;
work?
Or
const apiModel: ApiModel = {
currency,
addresses,
};
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.
To take it a step further, does this work?
wallet.component.html
onGetTransactions(transactionsReq: ApiModel) {
this._apiService.getRawTransactions(transactionsReq);
}
wallet.component.html
<button mat-raised-button (click)="onGetTransactions({ currency: currency.value, addresses: [addressOne.value, addressTwo.value]})">Get Transactions</button>
I am not sure if/how angular/typescript handle type errors in a template?
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.
As discussed we should do as ApiModel
for consistency.
const apiModel: ApiModel = {} as ApiModel;
|
||
<br> | ||
|
||
<input matInput placeholder="Enter Address" type="text" #addressTwo> |
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.
We still have a few matInput
, mat-raised
etc in the template files
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
@zanuka - Let us know when you are ready for a review (e.g. you think its completed) |
the following spec tests have been added for parsed response data:
|
@ro-savage it's ready for review. I'm happy with it at this point. From this pattern we can expand/adapt functionality. Just need to get it passing the lighthouse gauntlet 🔨 |
Codecov Report
@@ Coverage Diff @@
## master #46 +/- ##
=========================================
Coverage ? 84.07%
=========================================
Files ? 13
Lines ? 157
Branches ? 0
=========================================
Hits ? 132
Misses ? 25
Partials ? 0
Continue to review full report at Codecov.
|
txAddress.address = item['address']; | ||
txAddress.transactions = []; | ||
|
||
item['transactions'].forEach((tx) => { |
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.
Throws on item.transactions = null
which is the default result if only one item is requested.
Maybe should add a test for null/undefined, and ensure we handle it.
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.
@ro-savage sounds good, will add
@@ -28,8 +28,7 @@ | |||
"interface-over-type-literal": true, | |||
"label-position": true, | |||
"max-line-length": [ | |||
true, | |||
140 | |||
false | |||
], | |||
"member-access": false, | |||
"member-ordering": [ |
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.
On line 73 can we change :
"no-unused-expressions": true,
to
"no-unused-expressions": ["error", { "allowShortCircuit": true, "allowTernary": true }],
So we can do short cuts like data.transactions && data.transactions.forEach()
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.
great suggestion, just updated 🔍
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.
Happy with this as is.
Awesome work getting it started and getting us a great base to learn from.
We'll make whatever changes we need as we add more real world use cases.
@ro-savage sounds good, thanks for the review guys. Agreed, it's a good base and am looking forward to the next phase of feature adds. |
This wires up the api service, data service, model and wallet component to get raw tx data for supplied wallet addresses of selected currency.
Note: To test this, you'll need the Kauri API running. Follow the directions here