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

Bandwidth should be completely removed from code #3029

Closed
Smittyvb opened this issue Oct 3, 2018 · 7 comments
Closed

Bandwidth should be completely removed from code #3029

Smittyvb opened this issue Oct 3, 2018 · 7 comments

Comments

@Smittyvb
Copy link
Contributor

@Smittyvb Smittyvb commented Oct 3, 2018

Original:

Right now it's commented out, but IMO it should be completely removed.

AC:

Delete the bandwidth code in its entirety.
Remove bandwidth related API fields.
Document API changes.

@mvandeberg
Copy link
Contributor

@mvandeberg mvandeberg commented Oct 3, 2018

We agree completely. We made the decision to comment it out right now to get it done quickly with the plan to remove it later. Removing it requires a more delicate hand as APIs and other code rely on the bandwidth objects. The current version simply disables the calculations without handling the dependencies in the code that will take more time to remove.

@bobinson
Copy link

@bobinson bobinson commented Oct 3, 2018

Thank you @mvandeberg on your inputs.

We made the decision to comment it out right now to get it done quickly with the plan to remove it later.

Just to understand,

  1. are we planning to do a new version without completely removing the bandwidth code ?
  2. do we plan to test it extensively on the TESNET and make sure that all the side effects of removing the bandwidth code is identified and then, do a release ?
@reggaemuffin
Copy link

@reggaemuffin reggaemuffin commented Oct 8, 2018

I'm working on a patch for this. Almost ready, but I am writing tests to make sure nothing is breaking.

Branch: https://github.com/SteemCommunity/steem/tree/feature/remove-bandwith-completely

@reggaemuffin
Copy link

@reggaemuffin reggaemuffin commented Oct 8, 2018

SteemCommunity#3 is fixing this and the crashes, after review I will do a pr to the steemit repo. ping @theoreticalbts

@mvandeberg
Copy link
Contributor

@mvandeberg mvandeberg commented Oct 9, 2018

Need documentation of API changes from the removal.

@reggaemuffin
Copy link

@reggaemuffin reggaemuffin commented Oct 9, 2018

@mvandeberg I have decided for reasons of backwards compatibility to keep apis as is and return dummy values, so these endpoints can be deprecated.

Exact values returned are https://github.com/SteemCommunity/steem/pull/3/files#diff-f0a8047a921832891adab38f170b4580R25-45

I added messages to all code parts that need to be removed when deprecation happens to make it easier for the next developer.

@mvandeberg mvandeberg self-assigned this Oct 9, 2018
mvandeberg added a commit that referenced this issue Oct 9, 2018
mvandeberg added a commit that referenced this issue Oct 9, 2018
@mvandeberg
Copy link
Contributor

@mvandeberg mvandeberg commented Oct 10, 2018

We have discussed internally and decided to remove the deprecated APIs.

This will cause apps that rely on bandwidth to fail fast. Those apps might be misbehaving currently and failing silently.

The changed APIs are as follows:

  • condenser_api.get_dynamic_global_properties - Removed the following fields: average_block_size, current_reserve_ratio, max_virtual_bandwidth.

  • Removed the following fields from the extended_account_object: average_bandwidth, lifetime_bandwidth, last_bandwidth_update, average_market_bandwidth, lifetime_market_bandwidth, last_market_bandwidth_update.

  • Removed condenser_api. get_account_bandwidth.

  • Removed the witness_api.

@mac128k mac128k added this to Backlog (not prioritized) in Hardfork 20 (HF20/Velocity) Oct 10, 2018
@mac128k mac128k moved this from Backlog (not prioritized) to Done in Hardfork 20 (HF20/Velocity) Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants