Skip to content
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

Show the number of coins going in or out in a tx (address page) #74

Merged
merged 5 commits into from
Jan 17, 2018

Conversation

Senyoret1
Copy link
Contributor

Now the address screen shows the amount of coins that went in or out the address in each transaction. I think the green color shown when coins go into the address is very clear, but it is the one established in the Brand Guidelines. The amount of coins shows only up to 6 decimals. Issue involved: skycoin/skycoin#754
Sample of the boxes that were added to each transaction.

export function parseGetAddressTransaction(raw: GetAddressResponseTransaction, address: string): Transaction {

//Detect if the address sent or received the coins.
let incoming: boolean = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to type-cast it as a boolean, typescript will auto typecast as the default value is true


//Detect if the address sent or received the coins.
let incoming: boolean = true;
if (raw.inputs[0].owner.toLowerCase() == address.toLowerCase()) { incoming = false; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same line if statement does not require brackets

if (raw.inputs[0].owner.toLowerCase() == address.toLowerCase()) { incoming = false; }

//Calculate the amount of incoming or outgoing coins, as appropriate.
let coins:number = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

coins does not need to be cast as a number, will automatically be cast due to default value being a number

//Calculate the amount of incoming or outgoing coins, as appropriate.
let coins:number = 0;
for (let output of raw.outputs) {
if ((incoming == true) && (output.dst.toLowerCase() == address.toLowerCase())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Don't need to compare with true as incoming is already a boolean
  • Brackets are redundant

@@ -131,6 +150,7 @@ function parseGetBlocksTransaction(transaction: GetBlocksResponseBlockBodyTransa
inputs: transaction.inputs.map(input => ({ address: null, coins: null, hash: input, hours: null })),
outputs: transaction.outputs.map(output => parseGetBlocksOutput(output)),
status: null,
coins_change: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

When adding default values to be calculated always use null instead of 0

@@ -41,5 +41,6 @@ <h2>{{ address }}</h2>
</div>
</div>
</div>
<div class="row -row"><div attr.class="{{'coins_change_box '+(transaction.coins_change < 0 ? '-red' : '-green')}}">{{ (transaction.coins_change<0?"":"+")+transaction.coins_change.toString() }}</div></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Use NgClass

@@ -41,6 +41,7 @@ export class Transaction {
outputs: Output[];
status: boolean;
timestamp: number;
coins_change: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use balance or value instead of coins_change, I feel like this is more in line with the language used so far


//Detect if the address sent or received the coins.
let incoming: boolean = true;
if (raw.inputs[0].owner.toLowerCase() == address.toLowerCase()) { incoming = false; }
Copy link
Contributor

Choose a reason for hiding this comment

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

You have to check for each input, as the input could also be a second or later element in the array

Copy link
Contributor

@montycrypto montycrypto left a comment

Choose a reason for hiding this comment

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

Hi Senyoret, thanks for the pull request, I like how you expanded on the existing code. Please take a look at my comments, thank you!

@Senyoret1
Copy link
Contributor Author

Hello, thank you very much for the comments, I had doubts about the style I should use and now I understand better how to style the code. Additionally, I tried to make all the changes, but there was a problem: at first I assumed wrongly that all the inputs of the transactions come from the same address but in reality the inputs could come from multiple addresses. Because of that and the fact that the api only indicates the id and owner of each input (without indicating the amount of coins) it is only possible to know how many coins entered or left the account in each transaction by making a call to /api/uxout for each input of every transaction, which could have a very negative effect on performance. Because of that, I eliminated the sum of incoming-outgoing coins and simply placed a box that indicates whether coins entered or exited during the transaction. It is not as good as the amount of coins, but at least it helps to better understand the content of the screen. Here is a screenshot of how the new box looks like:
tx

@gz-c
Copy link
Member

gz-c commented Jan 17, 2018

@Senyoret1 can you describe what API data would be necessary to display in/out flows?

@samuelvisscher
Copy link
Contributor

Greatly improved, thanks! Please create an issue for the required API change

@samuelvisscher samuelvisscher merged commit f6d925b into skycoin:dev Jan 17, 2018
@Senyoret1 Senyoret1 deleted the incoming-or-outgoing branch June 4, 2018 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants