Skip to content

ETCM-[165,166]: Move RLP and crypto into their own sub-projects #823

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Dec 16, 2020
Merged

ETCM-[165,166]: Move RLP and crypto into their own sub-projects #823

merged 13 commits into from
Dec 16, 2020

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Dec 1, 2020 •
edited
Loading

Description

Separates out the rlp and crypto packages into their own sbt sub-projects within the same build, so that at some point they can be published as standalone libraries. Because they both depended on ByteUtils a separate bytes project was also created.

Dependencies were cut down to the minimum, but Akka remains - I think RLP the formats that got to do with ByteString could be moved to the node at some point, but it seems more pervasive in crypto.

The node jar keeps the mantis name so the Universal package as well as the Hello.id stays the same. Added the io.iohk org.

$ ls mantis-3.1.0/lib | grep io.iohk
io.iohk.mantis-3.1.0.jar
io.iohk.mantis-3.1.0-launcher.jar
io.iohk.mantis-bytes-3.1.0.jar
io.iohk.mantis-crypto-3.1.0.jar
io.iohk.mantis-rlp-3.1.0.jar
io.iohk.scalanet_2.12-0.4.4-SNAPSHOT.jar
io.iohk.scalanet-discovery_2.12-0.4.4-SNAPSHOT.jar

Also fixes coverage report paths, based on the logs these are the correct ones, but it could be different on buildkite:

[info] Wrote instrumentation file [/home/aakoshh/projects/iohk/mantis/target/scala-2.12/scoverage-data/scoverage.coverage]
[info] Will write measurement data to [/home/aakoshh/projects/iohk/mantis/target/scala-2.12/scoverage-data]

@aakoshh aakoshh force-pushed the ETCM-165-separate-rlp-sbt-project branch from 8ef73f5 to 1c112cc Compare December 1, 2020 16:27
@aakoshh aakoshh force-pushed the ETCM-165-separate-rlp-sbt-project branch from 1c112cc to 407926e Compare December 1, 2020 16:37
@aakoshh aakoshh requested a review from ntallar December 1, 2020 16:41
@aakoshh aakoshh changed the title ETCM-165: Move rlp to its own sub-project. ETCM-165: Move RLP to its own sub-project. Dec 1, 2020
lemastero
lemastero reviewed Dec 7, 2020
View reviewed changes
lemastero
lemastero reviewed Dec 7, 2020
View reviewed changes
rlp/src/main/scala/io/iohk/ethereum/rlp/RLPImplicits.scala Outdated
@@ -51,7 +50,7 @@ object RLPImplicits {
implicit val bigIntEncDec = new RLPEncoder[BigInt] with RLPDecoder[BigInt] {

override def encode(obj: BigInt): RLPValue = RLPValue(
if (obj.equals(BigInt(0))) byteToByteArray(0: Byte) else obj.toUnsignedByteArray
if (obj.equals(BigInt(0))) byteToByteArray(0: Byte) else bigIntToUnsignedByteArray(obj)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like toUnsignedByteArray in BigIntExtensionMethods.BigIntAsUnsigned
has the same implementation as: bigIntToUnsignedByteArray in RLP

Is his neccessary?

Copy link
Contributor Author

@aakoshh aakoshh Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It is indeed a carbon copy. I brought it over because this was the only thing it used from the utils, and I didn't want to create a "mantis-utils" library for RLP to depend on, and it also didn't look like the entire ByteUtils should be be moved to RLP. But I'm open to suggestions.

Copy link
Contributor Author

@aakoshh aakoshh Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe a dedicated mantis-bytes library? Probably crypto would use it too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a bytes project, moved bigIntToUnsignedByteArray to ByteUtils, and now BigIntExtensionMethods calls that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did the same treatment for crypto.

@aakoshh aakoshh changed the title ETCM-165: Move RLP to its own sub-project. ETCM-[165,166]: Move RLP and crypto into their own sub-projects Dec 7, 2020
@aakoshh aakoshh requested a review from KonradStaniec December 7, 2020 20:30
@aakoshh aakoshh force-pushed the ETCM-165-separate-rlp-sbt-project branch from 7a02080 to 6f083f5 Compare December 7, 2020 21:19
@aakoshh aakoshh requested a review from lemastero December 8, 2020 13:35
@aakoshh aakoshh requested a review from jmendiola222 December 10, 2020 16:54
@ntallar ntallar removed their request for review December 14, 2020 18:24
@aakoshh aakoshh assigned KonradStaniec and unassigned ntallar Dec 15, 2020
@aakoshh aakoshh merged commit 07ce3f6 into develop Dec 16, 2020
@aakoshh aakoshh deleted the ETCM-165-separate-rlp-sbt-project branch December 16, 2020 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enriquerodbe enriquerodbe enriquerodbe approved these changes

@KonradStaniec KonradStaniec Awaiting requested review from KonradStaniec

@lemastero lemastero Awaiting requested review from lemastero

@jmendiola222 jmendiola222 Awaiting requested review from jmendiola222

@biandratti biandratti Awaiting requested review from biandratti

Assignees

@KonradStaniec KonradStaniec

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aakoshh @enriquerodbe @lemastero @KonradStaniec @ntallar