Skip to content
This repository has been archived by the owner on Apr 4, 2022. It is now read-only.

WIP: #239 #248

Closed
wants to merge 14 commits into from
Closed

WIP: #239 #248

wants to merge 14 commits into from

Conversation

sa3664
Copy link

@sa3664 sa3664 commented Apr 10, 2021

Could you please review the code for Balance Functionality from the FE perspective?

Copy link
Contributor

@tboeckmann tboeckmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for checking in on this. I added some comments- happy to hear other suggestions too.

@todo
Copy link

todo bot commented Jun 26, 2021

handle failure

// TODO handle failure
).toPromise();
return res["body"]["balance"]
}


This comment was generated by todo based on a TODO comment in 59c8e78 in #248. cc @sa3664.

Copy link
Contributor

@tboeckmann tboeckmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making changes to the points from before.
I have queried a few elements to discuss further.

Also, you may need to add a couple more tests. Coverage has dropped.

@@ -15,6 +17,7 @@ import { ModalConnectivityErrorComponent } from 'src/app/modals/modal-connectivi

import * as ClassicEditor from '@ckeditor/ckeditor5-build-classic';
import { CKEditorComponent } from '@ckeditor/ckeditor5-angular';
import { getCurrencySymbol } from '@angular/common';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this used?

"type": 4,
"body": {
"balance": 2183846200000000000,
"unit": "wei"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check the unit? The API should return EDG for Edgeware

* Returns the currency on basis of the selected protocol in Mailchain
* @param protocol the cureent protocol
*/
getCurrencyForProtocol(protocol: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better for the API to return the currency based on the protocol and network? (e.g. wei for current Ethereum supported networks, edg for Edgeware-mainnnet, algo for Algorand)
This reduces the amount of logic required in the front end, although some is still require for UX, for example a wei to eth conversion for Ethereum.

"type": 4,
"body": {
"balance": 218,
"unit": "ALGO"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also check the case of the server response?

<br />
<!-- ./Show Balance Label -->
<!-- Show Fees Label -->
<!-- <div class="form-group row mb-0">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can you remove this until it's implemented?

@sa3664
Copy link
Author

sa3664 commented Sep 24, 2021

Thanks for sharing , I am working on the changes .

@robdefeo robdefeo closed this Apr 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants