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

feat: broadcast to multiple peers #937

Merged
merged 20 commits into from Jan 14, 2019

Conversation

@ItsANameToo
Copy link
Collaborator

commented Jan 5, 2019

Proposed changes

Enables switching between broadcasting to multiple peers or sending to just the connected peer (current behaviour)

Types of changes

  • New feature (non-breaking change which adds functionality)

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
@zillionn zillionn referenced this pull request Jan 8, 2019
1 of 12 tasks complete
@codecov-io

This comment has been minimized.

Copy link

commented Jan 11, 2019

Codecov Report

Merging #937 into develop will decrease coverage by 0.25%.
The diff coverage is 15.85%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #937      +/-   ##
===========================================
- Coverage    37.93%   37.67%   -0.26%     
===========================================
  Files          194      194              
  Lines         4626     4695      +69     
  Branches       889      903      +14     
===========================================
+ Hits          1755     1769      +14     
- Misses        2758     2808      +50     
- Partials       113      118       +5
Impacted Files Coverage Δ
...enderer/components/Transaction/TransactionShow.vue 43.75% <ø> (-1.42%) ⬇️
...llet/WalletHeading/WalletHeadingPrimaryActions.vue 46.66% <ø> (ø) ⬆️
.../components/Wallet/WalletDetails/WalletDetails.vue 10% <0%> (ø) ⬆️
...components/App/AppSidemenu/AppSidemenuSettings.vue 0% <0%> (ø) ⬆️
src/renderer/models/profile.js 0% <0%> (ø) ⬆️
src/renderer/services/client.js 66.21% <0%> (-5.93%) ⬇️
...nderer/components/Transaction/TransactionModal.vue 33.33% <15.78%> (-6.67%) ⬇️
src/renderer/store/modules/session.js 31.11% <20%> (-0.66%) ⬇️
src/renderer/store/modules/peer.js 71.29% <37.5%> (-2.23%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c51899...625642e. Read the comment docs.

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

It doesn't close the popup for me after I send the tx.

@ItsANameToo

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 11, 2019

Yes, there were some changes on the develop branch that change the mechanic

@dated

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

:hide:

@ItsANameToo

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 11, 2019

always the same people :colbert:

ItsANameToo added some commits Jan 11, 2019

@ItsANameToo ItsANameToo changed the title [WIP] feat: broadcast to multiple peers feat: broadcast to multiple peers Jan 11, 2019

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

Did you integrate my changes here #841 ? I sent tx with 300 fee and got error message, even though the tx was broadcasted:

screenshot 2019-01-11 at 18 08 23

{"data":{"accept":[],"broadcast":["72a83c813232f6fb0cd2638de32114a49000d19ecba3308f4504ef6519794680"],"excess":[],"invalid":["72a83c813232f6fb0cd2638de32114a49000d19ecba3308f4504ef6519794680"]},"errors":{"72a83c813232f6fb0cd2638de32114a49000d19ecba3308f4504ef6519794680":[{"type":"ERR_LOW_FEE","message":"Too low fee to be accepted in the pool"}]}}

@ItsANameToo

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 11, 2019

@zillionn the aim of this PR was to add broadcasting to multiple peers. This does not include the changes you proposed in #841 as I believe that's not in the scope of this PR.

Why did you close your PR? I think it's a good addition, but it's not part of this PR 🤔

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

I thought you're going to fix this and to be honest, I don't want to create PRs anymore.

However, with the latest core/develop code, which doesn't return invalid and error anymore, your PR works fine and shows warning:

screenshot 2019-01-11 at 18 17 11

But it'll probably break on mainnet.

@alexbarnsley
Copy link
Member

left a comment

A few little tweaks

const { data, errors } = response.data
if (responseArray.length > 0) {
let i
for (i = 0; i < responseArray.length; i++) {

This comment has been minimized.

Copy link
@alexbarnsley

alexbarnsley Jan 14, 2019

Member

This could be for (let i = 0; ...

success = false
} else {
// no tx was sent

This comment has been minimized.

Copy link
@alexbarnsley

alexbarnsley Jan 14, 2019

Member

Probably don't need this comment since the message name nothingSent is self explanatory.

const peers = store.getters['peer/broadcastPeers']()

let i
for (i = 0; i < peers.length; i++) {

This comment has been minimized.

Copy link
@alexbarnsley

alexbarnsley Jan 14, 2019

Member

for (let i = 0; ...

const peers = store.getters['peer/broadcastPeers']()

let i
for (i = 0; i < peers.length; i++) {

This comment has been minimized.

Copy link
@alexbarnsley

alexbarnsley Jan 14, 2019

Member

for (let i = 0; ...

/**
* Broadcast transactions to the current peer.
*
* @param {Array|Object} transactions
* @param {Boolean} broadcast - whether the transaction should be broadcasted to multiple peers or not
* @returns {Object}

This comment has been minimized.

Copy link
@alexbarnsley

alexbarnsley Jan 14, 2019

Member

@returns {Object[]}


const peers = config.PEERS[networkId]
if (!peers || !peers.length) {
return null

This comment has been minimized.

Copy link
@alexbarnsley

alexbarnsley Jan 14, 2019

Member

Maybe return empty array instead of null, to match the earlier case of no network and no profile

@ItsANameToo

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 14, 2019

You can test a final time @alexbarnsley

@alexbarnsley alexbarnsley merged commit e7bdea9 into develop Jan 14, 2019

1 check passed

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

@alexbarnsley alexbarnsley deleted the feat/broadcast-tx branch Jan 14, 2019

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