This repository has been archived by the owner on Sep 26, 2019. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 130
RLP encode subcommand #944
Merged
NicolasMassart
merged 10 commits into
PegaSysEng:master
from
NicolasMassart:feature/PAN-2318_rlp_encode_subcommand
Feb 25, 2019
Merged
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
ee8a1db
wip
NicolasMassart 66bd7a6
Merge branch 'master' into feature/PAN-2318_rlp_encode_subcommand
NicolasMassart 04e2816
Add RLP encoding subcommand for ibftExtraData to be inserted in genes…
NicolasMassart 0a1db47
Updated code to use an adapter instead of modifying the deep classes …
NicolasMassart 2da0791
Added CLI ref doc
NicolasMassart 3fa8550
Add more test and change an error type to parameter error
NicolasMassart 9439640
remove some useless changes and added doc
NicolasMassart be17af4
updated to enable specific implementation of json mapping for each type
NicolasMassart ba7677c
Merge branch 'master' into feature/PAN-2318_rlp_encode_subcommand
NicolasMassart 20668a9
Merge branch 'master' into feature/PAN-2318_rlp_encode_subcommand
NicolasMassart File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
47 changes: 47 additions & 0 deletions
47
pantheon/src/main/java/tech/pegasys/pantheon/cli/rlp/IbftExtraDataCLIAdapter.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
/* | ||
* Copyright 2019 ConsenSys AG. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on | ||
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations under the License. | ||
*/ | ||
package tech.pegasys.pantheon.cli.rlp; | ||
|
||
import tech.pegasys.pantheon.consensus.ibft.IbftExtraData; | ||
import tech.pegasys.pantheon.ethereum.core.Address; | ||
import tech.pegasys.pantheon.util.bytes.BytesValue; | ||
|
||
import java.io.IOException; | ||
import java.util.Collection; | ||
import java.util.Collections; | ||
import java.util.Optional; | ||
import java.util.stream.Collectors; | ||
|
||
import com.fasterxml.jackson.core.type.TypeReference; | ||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
|
||
/** | ||
* Adapter to convert a typed JSON to an IbftExtraData object This adapter handles the JSON to RLP | ||
* encoding | ||
*/ | ||
public class IbftExtraDataCLIAdapter implements JSONToRLP { | ||
|
||
@Override | ||
public BytesValue encode(final String json) throws IOException { | ||
ObjectMapper mapper = new ObjectMapper(); | ||
TypeReference<Collection<String>> typeRef = new TypeReference<Collection<String>>() {}; | ||
Collection<String> validatorAddresse = mapper.readValue(json, typeRef); | ||
|
||
Collection<Address> addresses = | ||
validatorAddresse.stream().map(Address::fromHexString).collect(Collectors.toList()); | ||
|
||
return new IbftExtraData( | ||
BytesValue.wrap(new byte[32]), Collections.emptyList(), Optional.empty(), 0, addresses) | ||
.encode(); | ||
} | ||
} |
28 changes: 28 additions & 0 deletions
28
pantheon/src/main/java/tech/pegasys/pantheon/cli/rlp/JSONToRLP.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
/* | ||
* Copyright 2019 ConsenSys AG. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on | ||
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations under the License. | ||
*/ | ||
package tech.pegasys.pantheon.cli.rlp; | ||
|
||
import tech.pegasys.pantheon.util.bytes.BytesValue; | ||
|
||
import java.io.IOException; | ||
|
||
/** Behaviour of objects that can be encoded from JSON to RLP */ | ||
interface JSONToRLP { | ||
|
||
/** | ||
* Encodes the object into an RLP value. | ||
* | ||
* @return the RLP encoded object. | ||
*/ | ||
BytesValue encode(String json) throws IOException; | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of an extra interface with an adapter, I think it would be more concise and direct to move the method body into the enum type and add an "encode" method on that enum:
shemnon@e178baf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that for now we only have one type in the enum but we may end up with more and I wanted to keep the enum as simple as possible by just referencing an adapter and have one adapter for each data type, well isolated into each class file. But why not. What would you think @mbaxter and @rain-on ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the same brace-initialized-enum-value pattern for future encoders, so this doesn't preclude more values in the enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it doesn't but the more values we'll have in the enum the more code we'll have and I'm happier with small files when possible. But I admit your proposal removes a class and an interface so why not. Waiting for a second advice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keeping the enum simple makes sense to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed I would like to try to start splitting things to prevent mega classes like PantheonCommand. CLI is usually the place where you enter Pantheon so having it very easy to navigate seems important to me. So yes it requires a bit more classes and interfaces but it also makes it easier for someone new to understand and add a feature in my opinion.