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

Fix special transactions that were incorrectly colored as incoming #146

Merged
merged 1 commit into from May 8, 2018

Conversation

Projects
None yet
3 participants
@ItsANameToo
Copy link
Contributor

ItsANameToo commented May 7, 2018

This adds an extra check on transaction.type when providing a color to a transaction. Originally, special transactions such as votes, signatures- and delegate registrations were given a green color, as the recipient corresponded to the current wallet. However, they are actually outgoing transactions (and are also shown on the 'sent' page) and should therefore be indicated in red.

@faustbrian faustbrian merged commit 191c39f into ArkEcosystem:master May 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ItsANameToo ItsANameToo deleted the ItsANameToo:fix-transaction-color branch May 10, 2018

@geopsllc

This comment has been minimized.

Copy link
Contributor

geopsllc commented May 25, 2018

This change breaks mobile pages - transactions are not shown at all. Also, votes are still counted as both incoming and outgoing in the transactions field.

@ItsANameToo

This comment has been minimized.

Copy link
Contributor Author

ItsANameToo commented May 25, 2018

This change is live on explorer.ark.io and works as intended. If you are having issues on your fork, it might be because it is 68 commits behind and some other things are interfering.

Do you have any further information on the error that you get when applying the change from this PR? I don't see how it would break mobile pages as it simply adds a check for special transaction types.

@geopsllc

This comment has been minimized.

Copy link
Contributor

geopsllc commented May 25, 2018

I see that it is live now and works on the ark explorer. Not sure why but it doesn't work on my fork. Works fine on desktop just not on mobile. Transactions are not showing up at all on mobile. Removing the additional check from the 2 mobile vue files start showing them again but with the wrong colors. Even on desktop the counts for in and out are still wrong but i guess that's calculated somewhere else.

@ItsANameToo

This comment has been minimized.

Copy link
Contributor Author

ItsANameToo commented May 25, 2018

Looking at your commit, you made some mistakes when adding the changes to your fork. You used :type="row.type" in all the files, while the mobile/transactions.vue and mobile/transactionsDetail need :type="transaction.type" instead.

That would at least explain why it gives you issues on mobile

@geopsllc

This comment has been minimized.

Copy link
Contributor

geopsllc commented May 25, 2018

You are correct - I am a moron :)

@ItsANameToo

This comment has been minimized.

Copy link
Contributor Author

ItsANameToo commented May 25, 2018

Not at all, it's easy to overlook small things 😛

Regarding the counts for in and out, do you mean that votes are shown for both 'sent' and 'received'? Because I see that that is also an issue for the ark explorer then.

@geopsllc

This comment has been minimized.

Copy link
Contributor

geopsllc commented May 25, 2018

Yup. The counter is counting them as both. Not sure where that calculation happens though.

@ItsANameToo

This comment has been minimized.

Copy link
Contributor Author

ItsANameToo commented May 25, 2018

Received Transactions are fetched here, so you could filter the response data to remove votes (transaction type is 3). That way you will not only remove them from being counted, but also from the "received" tab in a wallet :)

@geopsllc

This comment has been minimized.

Copy link
Contributor

geopsllc commented May 25, 2018

Thanks. I will try but I'm not sure I can figure it out. No clue how to add a filter by tx type. JS is not my thing :)

@ItsANameToo

This comment has been minimized.

Copy link
Contributor Author

ItsANameToo commented May 25, 2018

Change the line to .then(response => response.data.transactions.filter(transaction => transaction.type != 3)) and it will filter out the votes. However, I saw that this only removes it from the "received" tab, but are still counted as "received"..

Will take a closer look where the count is based on.

@geopsllc

This comment has been minimized.

Copy link
Contributor

geopsllc commented May 25, 2018

Thanks. Maybe you can fix that for ark and push a commit when you have the time :)

@ItsANameToo

This comment has been minimized.

Copy link
Contributor Author

ItsANameToo commented May 25, 2018

This function fetches the count, but at that point you cannot filter the transactions as we do not have those available (and some addresses will have hundreds of transactions that you would have to go through to filter the count correctly). Not sure whether there is a simple way to correctly show the count with the current API

@geopsllc

This comment has been minimized.

Copy link
Contributor

geopsllc commented May 25, 2018

OK, thanks anyway. You're awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment