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

fix: capture any error when sending transactions and warn about broadcasting #829

Merged
merged 7 commits into from Dec 21, 2018

Conversation

Projects
None yet
4 participants
@j-a-m-l
Copy link
Contributor

commented Dec 21, 2018

Proposed changes

This PR fixes 1 problem introduced on #820, which ignores that the API response could include other errors.

Apart from that, it displays a warning when the transaction is only broadcasted.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Test (adding missing tests or fixing existing tests)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

j-a-m-l added some commits Dec 21, 2018

@j-a-m-l j-a-m-l changed the title [WIP] fix: capture any error when sending transactions and warn about broadcasting fix: capture any error when sending transactions and warn about broadcasting Dec 21, 2018

@j-a-m-l j-a-m-l requested review from alexbarnsley and luciorubeens Dec 21, 2018

alexbarnsley and others added some commits Dec 21, 2018

Merge branch 'fix-fix-of-transaction-responses' of github.com:ArkEcos…
…ystem/desktop-wallet into fix-fix-of-transaction-responses

@alexbarnsley alexbarnsley merged commit c29a9c5 into develop Dec 21, 2018

1 check passed

ci/circleci: test-node-9 Your tests passed on CircleCI!
Details

@j-a-m-l j-a-m-l deleted the fix-fix-of-transaction-responses branch Dec 21, 2018

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

I didn't do this broadcast warning message, because imo most users have no idea what accept and what broadcast means. They just care if the tx is going through or not. Most have no idea what Transaction was broadcasted to other peers. It may not be accepted by them means.

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

Maybe more clear message that the problem is with the fee, something like:
Transaction was successfully sent but it may not be accepted/forged because the fee (Ѧ0.00000300) is too low.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

@zillionn I'm going to add more changes and tests to this component, how would you rephrase the broadcast warning?

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

Maybe something like this:
Transaction was successfully sent but it may not be accepted/forged because the fee (Ѧ0.00000300) is too low.

I'm also experiencing something else. I'm using custom peer 95.179.150.18:4003 and I'm pulling the data from it, but when I send a tx, it is sent to a different peer 167.114.29.34:4003:
screenshot 2018-12-21 at 18 00 22
screenshot 2018-12-21 at 18 00 33
screenshot 2018-12-21 at 18 00 43

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

I guess the problem is with alternativeWallet, it's always set in onBuilt:

onBuilt ({ transaction, wallet }) {
this.step = 1
this.transaction = transaction
this.alternativeWallet = wallet
},

@dated

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

The wallet looks for the best peer and sends to that.

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

It should use the connected peer. It uses best/other only if you're using alternativeWallet in the send modal:

if (this.alternativeWallet) {
const peer = await this.$store.dispatch('peer/findBest', {
refresh: true,
network: this.walletNetwork
})
const apiClient = await this.$store.dispatch('peer/clientServiceFromPeer', peer)
response = await apiClient.broadcastTransaction(this.transaction)
} else {
response = await this.$client.broadcastTransaction(this.transaction)
}

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

@j-a-m-l Also your code is wrong and breaks what I did:

return data && data.invalid.length === 0 && errors === null

If my tx is broadcasted but not accepted:

10|ark-core-relay | [2018-12-21 16:30:23][DEBUG] : Transaction bb2f15321ddfa051decd081e98984a070100e35160185e5b140d63aae543e017 eligible for broadcast - fee of 0.000003 DѦ is greater than minimum fee (0.00000253 DѦ)
10|ark-core-relay | [2018-12-21 16:30:23][DEBUG] : Transaction bb2f15321ddfa051decd081e98984a070100e35160185e5b140d63aae543e017 not eligible to enter pool - fee of 0.000003 DѦ is smaller than minimum fee (0.00759 DѦ)
10|ark-core-relay | [2018-12-21 16:30:23][INFO] : Received 1 transaction (accept: 0 broadcast: 1 excess: 0 invalid: 1).

the above code should return true because the tx is broadcasted, but it returns false, because data.invalid.length != 0 and errors != null.
screenshot 2018-12-21 at 18 34 57

You can have invalid and errors but tx to go through.

I believe my check is the better approach:
https://github.com/zillionn/desktop-wallet/blob/94733b6d1a3d8116ffd142285629c437f925b3ef/src/renderer/components/Transaction/TransactionModal.vue#L170-L172

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.