Skip to content
This repository has been archived by the owner on Mar 3, 2020. It is now read-only.

[KEP-38] Getting token count from Ropsten network + unit test. #5

Closed
wants to merge 8 commits into from

Conversation

dmitrymukhin
Copy link
Contributor

Added ethereum_api class and token_balance() method to pull balance for specific token on specific account. Added unit test to check balance on well known account.

@rnistuk
Copy link
Contributor

rnistuk commented Nov 9, 2017

  • 13 unit tests pass
  • there are log messages that need to be removed (probably from a different set of commits):
Node created: 247.0.234.115
Node destroyed: 247.0.234.115
Node created: 249.189.30.45
Node destroyed: 249.189.30.45
Node created: 230.135.179.32
Node destroyed: 230.135.179.32

Let's get rid of those now.

  • ethereum_api::token_balance is a bit large and there are blocks of code that can bet turned into functions to make it easier to read.
  • Returning -1.0 is pretty arbitrary, would throwing a custom exception be better here?
  • ethereum_token uses std::stringstream but does not include the sstream header. Let's not rely on accidents to make the project build.
  • the format of ethereum_api does not follow Bluzelle coding guidelines.

@rnistuk rnistuk closed this Nov 9, 2017
@rnistuk rnistuk changed the base branch from master to devel November 9, 2017 19:45
@rnistuk rnistuk reopened this Nov 9, 2017
@rnistuk
Copy link
Contributor

rnistuk commented Nov 9, 2017

Accidentally closed the request. Reopening and setting the base to develop.

@rnistuk
Copy link
Contributor

rnistuk commented Nov 9, 2017

  • please add a test for failure of the etherium_api
  • the tests should follow the bluzelle coding guidelines.

@rnistuk
Copy link
Contributor

rnistuk commented Nov 9, 2017

CMakeLists.txt

  • macOS doesn't need the pthread library to be linked. Wrap those directives in if(UNIX AND NOT APPLE) blocks

@dmitrymukhin
Copy link
Contributor Author

@rnistuk Cool, will fix shortly.

Copy link
Contributor

@rnistuk rnistuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments.

@dmitrymukhin
Copy link
Contributor Author

Reformatted
Fixed makefile
Split into smaller methods
added check for failure UT
Include missing header.

@rnistuk Could you take a look?

@rnistuk
Copy link
Contributor

rnistuk commented Nov 10, 2017

  • Node::generate_name, on_death, on_birth are only used by node, is there a reason they should not be private?
  • In Node::generate_name you use boost::random::random_device, which is not available on all systems (http://www.boost.org/doc/libs/1_65_1/doc/html/boost/random/random_device.html). Use std::time(0) instead to initialize the mt19937
  • I've been thinking about protected vs. private, and I think that to really enforce encapsulation we need to favour private, and then when required, move methods to protected only when really necessary. We have no idea if we are going to extend the class at this time so why break encapsulation early?
  • in EthereumToken why do we need address_ and decimal_points_ to be public? They should really be exposed with const getters as there is no reason to change them after instantiation.
  • Node::generate_name should be unit tested by creating a node and verifying that Node::name returns a string who's format can be checked with regex. Ideally it should also be valid public IPv4, but checkin the format should be sufficient.
  • since you can inject the listener, on_birth and on_death should also be unit tested.

@rnistuk
Copy link
Contributor

rnistuk commented Nov 15, 2017

  • 16 unit tests pass, good
  • EtheriumAPI.h not following Whitsmith style
  • EtheriumAPI connect_socket(), write_request, read_response, parse_response.. should be private unless the class is being extended, we should not solve future problems that may never exist.

@dmitrymukhin
Copy link
Contributor Author

@rnistuk I fixed EthereumApi.h any more comments?

@rnistuk
Copy link
Contributor

rnistuk commented Nov 15, 2017

Great thanks.

@rnistuk rnistuk closed this Nov 15, 2017
bluzelle pushed a commit that referenced this pull request Dec 13, 2017
# This is the 1st commit message:

Implemented node Storage with CRUD interface.

# This is the commit message #2:

[KEP-15] Class hierachy draft.

# This is the commit message #3:

[KEP-15] Notes on leader election, protocol specs, Node::get_peers() and Node::get_messages() headers.

# This is the commit message #4:

[KEP-15] Node able to receive heartbeats.

# This is the commit message #5:

[KEP-15] Leaders heartbeat timer working.

# This is the commit message #6:

[KEP-15] Interim commit, trying to send heartbeat message to peers.

# This is the commit message #7:

[KEP-15] Interim commit, trying to send heartbeat message to peers. Take 2.

# This is the commit message #8:

[KEP-15] Heartbeat messages sent, but only synchronously.

# This is the commit message #9:

[KEP-15] Starting CRUD impl.

# This is the commit message #10:

[KEP-15] Draft of CRUD support + storage as map.

.idea/codeStyles

First attempt at serialization and deserialization of node data.

Implemented CRUD and unit tests for Node Storage.

removed code styles files from the repo.

Removed a duplicate default parameter in the create_random_value function definition.

Cleaned up Storage and unit tests for review.
bluzelle pushed a commit that referenced this pull request Dec 13, 2017
# This is the 1st commit message:

Implemented node Storage with CRUD interface.

# This is the commit message #2:

[KEP-15] Class hierachy draft.

# This is the commit message #3:

[KEP-15] Notes on leader election, protocol specs, Node::get_peers() and Node::get_messages() headers.

# This is the commit message #4:

[KEP-15] Node able to receive heartbeats.

# This is the commit message #5:

[KEP-15] Leaders heartbeat timer working.

# This is the commit message #6:

[KEP-15] Interim commit, trying to send heartbeat message to peers.

# This is the commit message #7:

[KEP-15] Interim commit, trying to send heartbeat message to peers. Take 2.

# This is the commit message #8:

[KEP-15] Heartbeat messages sent, but only synchronously.

# This is the commit message #9:

[KEP-15] Starting CRUD impl.

# This is the commit message #10:

[KEP-15] Draft of CRUD support + storage as map.

.idea/codeStyles

First attempt at serialization and deserialization of node data.

Implemented CRUD and unit tests for Node Storage.

removed code styles files from the repo.

Removed a duplicate default parameter in the create_random_value function definition.

Cleaned up Storage and unit tests for review.
@ebruck ebruck deleted the KEP-38 branch April 26, 2018 20:12
ebruck added a commit that referenced this pull request Apr 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants