Outputs ENR text representation in admin.nodeInfo RPC by twinstar26 · Pull Request #5701 · ethereum/aleth

twinstar26 · 2019-07-27T06:52:58Z

twinstar26 · 2019-07-27T06:55:36Z

@gumb0 based on reviews in old PR #5697

Everything seems to be working fine. Also can you double check changes in libdevcore/Base64.cpp.
If everything is fine I will go on to create a unit test as suggested.

gumb0

Looks good, just several minor stylistic changes needed.

Also, could you please in a separate commit (can be this same PR) convert all tabs to spaces in the file Base64.cpp?

And yes, a unit test would be appreciated.

gumb0 · 2019-07-29T16:40:33Z

libdevcore/Base64.cpp

 }

-string dev::toBase64(bytesConstRef _in)
+string toBase64Encoding(bytesConstRef _in, char const* base64_chars, bool _pad)


Suggested change

string toBase64Encoding(bytesConstRef _in, char const* base64_chars, bool _pad)

string toBase64Encoding(bytesConstRef _in, char const* _base64_chars, bool _pad)

gumb0 · 2019-07-29T16:41:05Z

libdevcore/Base64.cpp

+		"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+		"abcdefghijklmnopqrstuvwxyz"
+		"0123456789+/";
+	bool _pad = true;


Suggested change

bool _pad = true;

bool const pad = true;

gumb0 · 2019-07-29T16:41:19Z

libdevcore/Base64.cpp

+		"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+		"abcdefghijklmnopqrstuvwxyz"
+		"0123456789-_";
+	bool _pad = false;


Suggested change

bool _pad = false;

bool const pad = false;

gumb0 · 2019-07-29T16:41:43Z

libdevcore/Base64.cpp


+string dev::toBase64(bytesConstRef _in)
+{
+	static const char base64_chars[] =


Suggested change

static const char base64_chars[] =

static char const base64_chars[] =

gumb0 · 2019-07-29T16:41:54Z

libdevcore/Base64.cpp

+
+string dev::toBase64URLSafe(bytesConstRef _in)
+{
+	static const char base64_chars[] =


Suggested change

static const char base64_chars[] =

static char const base64_chars[] =

gumb0 · 2019-07-29T16:42:43Z

libp2p/ENR.cpp

+std::string ENR::textEncoding() const
+{
+    RLPStream s;
+    this->streamRLP(s);


Suggested change

this->streamRLP(s);

streamRLP(s);

gumb0 · 2019-07-29T16:48:17Z

libp2p/ENR.cpp

+{
+    RLPStream s;
+    this->streamRLP(s);
+    return toBase64URLSafe(std::string(s.out().begin(), s.out().end()));


I think something like

Suggested change

return toBase64URLSafe(std::string(s.out().begin(), s.out().end()));

return toBase64URLSafe(ref(s.out()));

should work

gumb0 · 2019-07-29T16:49:38Z

libp2p/Host.h

-    p2p::NodeInfo nodeInfo() const { return NodeInfo(id(), (networkConfig().publicIPAddress.empty() ? m_tcpPublic.address().to_string() : networkConfig().publicIPAddress), m_tcpPublic.port(), m_clientVersion); }
+    p2p::NodeInfo nodeInfo() const
+    { 
+        auto e = enr();


Suggested change

auto e = enr();

auto const e = enr();

gumb0

Ah, one more thing: you need to add a prefix enr: in function ENR::textEncoding()
See the spec https://github.com/ethereum/EIPs/blob/master/EIPS/eip-778.md#text-encoding

gumb0 · 2019-07-29T16:59:11Z

For unit test add a check that enr.textEncoding() is enr:-IS4QHCYrYZbAKWCBRlAy5zzaDZXJBGkcnh4MHcBFZntXNFrdvJjX04jRzjzCBOonrkTfj499SZuOh8R33Ls8RRcy5wBgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQPKY0yuDUmstAHYpMa2_oxVtw0RW_QAdpzBQA8yWM0xOIN1ZHCCdl8 to this test

aleth/test/unittests/libp2p/ENRTest.cpp

Line 24 in b212789

TEST(enr, parse)

(this is a test vector from EIP spec)

codecov-io · 2019-07-29T17:06:46Z

Codecov Report

Merging #5701 into master will decrease coverage by <.01%.
The diff coverage is 84.81%.

@@            Coverage Diff             @@
##           master    #5701      +/-   ##
==========================================
- Coverage   63.05%   63.05%   -0.01%     
==========================================
  Files         353      351       -2     
  Lines       30110    30015      -95     
  Branches     3378     3362      -16     
==========================================
- Hits        18987    18926      -61     
+ Misses       9894     9880      -14     
+ Partials     1229     1209      -20

twinstar26 · 2019-07-31T14:33:35Z

Ah, one more thing: you need to add a prefix enr: in function ENR::textEncoding()
See the spec https://github.com/ethereum/EIPs/blob/master/EIPS/eip-778.md#text-encoding

@gumb0
Thanks for the reviews!
So I was thinking of string concat of "enr:" and enrtext since ENR::textEncoding() returns string.

twinstar26 · 2019-07-31T14:36:27Z

For unit test add a check that enr.textEncoding() is enr:-IS4QHCYrYZbAKWCBRlAy5zzaDZXJBGkcnh4MHcBFZntXNFrdvJjX04jRzjzCBOonrkTfj499SZuOh8R33Ls8RRcy5wBgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQPKY0yuDUmstAHYpMa2_oxVtw0RW_QAdpzBQA8yWM0xOIN1ZHCCdl8 to this test

aleth/test/unittests/libp2p/ENRTest.cpp

Line 24 in b212789

TEST(enr, parse)

(this is a test vector from EIP spec)

@gumb0
So after this do I have to create a unit-test explicitly and if so what is that (output of which function) I have to test.

gumb0 · 2019-07-31T17:01:46Z

So I was thinking of string concat of "enr:" and enrtext since ENR::textEncoding() returns string.

Yes, just return "enr:" + toBase64URLSafe(...) from textEncoding()

So after this do I have to create a unit-test explicitly and if so what is that (output of which function) I have to test.

Let's add just this one check

EXPECT_EQ(enr.textEncoding(), "enr:-IS4QHCYrYZbAKWCBRlAy5zzaDZXJBGkcnh4MHcBFZntXNFrdvJjX04jRzjzCBOonrkTfj499SZuOh8R33Ls8RRcy5wBgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQPKY0yuDUmstAHYpMa2_oxVtw0RW_QAdpzBQA8yWM0xOIN1ZHCCdl8")

to the end of the test I linked, I think that would be enough for this PR.

gumb0 · 2019-07-31T17:02:55Z

Also please add an item to CHANGELOG.md

twinstar26 · 2019-08-02T12:12:33Z

@gumb0
This is merge ready. Please check and if not so please suggest changes.

gumb0

Looks good, please fix just one last minor naming issue and changelog, then it's ready

gumb0 · 2019-08-02T15:59:06Z

libdevcore/Base64.cpp

-	}
-
-	return ret;
+    static char const _base64_chars[] =


Suggested change

static char const _base64_chars[] =

static char const c_base64_chars[] =

gumb0 · 2019-08-02T15:59:29Z

libdevcore/Base64.cpp

+
+string dev::toBase64URLSafe(bytesConstRef _in)
+{
+    static char const _base64_chars[] =


Suggested change

static char const _base64_chars[] =

static char const c_base64_chars[] =

gumb0 · 2019-08-02T16:00:24Z

CHANGELOG.md

 - Added: [#5634](https://github.com/ethereum/aleth/pull/5634) Bootnodes for Rinkeby and Goerli.
 - Added: [#5640](https://github.com/ethereum/aleth/pull/5640) Istanbul support: EIP-1702 Generalized Account Versioning Scheme.
 - Added: [#5690](https://github.com/ethereum/aleth/issues/5690) Istanbul support: EIP-2028 transaction data gas cost reduction.
+- Added: [#5622](https://github.com/ethereum/aleth/issues/5622) Outputs ENR text representation in admin.nodeInfo RPC.


Please link this to this PR instead of an issue

minor fixes left

chfast · 2019-08-05T14:57:03Z

Merge?

gumb0 self-requested a review July 29, 2019 10:15

gumb0 mentioned this pull request Jul 29, 2019

Output ENR text representation in admin.nodeInfo RPC #5697

Closed

gumb0 suggested changes Jul 29, 2019

View reviewed changes

twinstar26 force-pushed the aleth-enrtext branch from b40faf5 to 9e54363 Compare July 31, 2019 18:41

gumb0 previously approved these changes Aug 2, 2019

View reviewed changes

gumb0 self-requested a review August 2, 2019 16:03

twinstar26 added 3 commits August 2, 2019 21:55

Outputs ENR text representation in admin.nodeInfo RPC

5be6d5f

Tabs to spaces

17b9f1b

added unit-test for outputing ENR text representation

542646b

twinstar26 force-pushed the aleth-enrtext branch from 9e54363 to 542646b Compare August 2, 2019 16:38

gumb0 added 2 commits August 5, 2019 10:01

Fix naming

6a66526

Return ENR in admin_nodeInfo RPC

ce7201f

gumb0 approved these changes Aug 5, 2019

View reviewed changes

gumb0 merged commit 88e6896 into ethereum:master Aug 5, 2019

	string toBase64Encoding(bytesConstRef _in, char const* base64_chars, bool _pad)
	string toBase64Encoding(bytesConstRef _in, char const* _base64_chars, bool _pad)

	static const char base64_chars[] =
	static char const base64_chars[] =

	return toBase64URLSafe(std::string(s.out().begin(), s.out().end()));
	return toBase64URLSafe(ref(s.out()));

	static char const _base64_chars[] =
	static char const c_base64_chars[] =

Conversation

twinstar26 commented Jul 27, 2019

Uh oh!

twinstar26 commented Jul 27, 2019

Uh oh!

gumb0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gumb0 left a comment

Choose a reason for hiding this comment

Uh oh!

gumb0 commented Jul 29, 2019

Uh oh!

codecov-io commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

twinstar26 commented Jul 31, 2019

Uh oh!

twinstar26 commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gumb0 commented Jul 31, 2019

Uh oh!

gumb0 commented Jul 31, 2019

Uh oh!

twinstar26 commented Aug 2, 2019

Uh oh!

gumb0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chfast commented Aug 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Jul 29, 2019 •

edited

Loading

twinstar26 commented Jul 31, 2019 •

edited

Loading