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

Move broadcast_transaction_synchronous to condenser_api #2317

Merged
merged 5 commits into from
May 1, 2018

Conversation

mvandeberg
Copy link
Contributor

Closes #2278

@theoreticalbts
Copy link
Contributor

theoreticalbts commented Apr 11, 2018

Merge conflict in network_broadcast_api.cpp. Also, I'm pretty sure this will not compile since #2325 was merged.

@mvandeberg
Copy link
Contributor Author

Will look into it.

@mvandeberg mvandeberg force-pushed the 2278-move-broadcast-tx-sync branch from b36c4cb to eb1daf0 Compare April 11, 2018 22:12
@mvandeberg
Copy link
Contributor Author

@theoreticalbts Conflicts have been resolved.

@theoreticalbts
Copy link
Contributor

It looks suspicious that the API call implementation parses code as legacy_signed_transaction, but the cli_wallet remote API declaration says signed_transaction.

Has cli_wallet transaction broadcast been tested?

Also, minor nitpick: Signal field/method are named on_applied_block, _on_applied_block_connection. But the signal refactoring standardized those names to on_post_apply_block, _post_apply_block_conn.

@mvandeberg
Copy link
Contributor Author

I'll double check the wallet.

We need to document such naming conventions. I noticed that there was some renaming because of the changed signal names, but I guess I did not notice the standardization of the names. I can make the change.

@theoreticalbts
Copy link
Contributor

Approved

@mvandeberg mvandeberg merged commit 00abf5d into develop May 1, 2018
@mvandeberg mvandeberg deleted the 2278-move-broadcast-tx-sync branch October 23, 2018 22:24
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.

2 participants