Skip to content

[rlp] Upgrade ethereum-types dependency#158

Merged
dvdplm merged 7 commits into
masterfrom
dp/chore/update-rlp-to-ethereum-types-0.5
May 23, 2019
Merged

[rlp] Upgrade ethereum-types dependency#158
dvdplm merged 7 commits into
masterfrom
dp/chore/update-rlp-to-ethereum-types-0.5

Conversation

@dvdplm
Copy link
Copy Markdown
Contributor

@dvdplm dvdplm commented May 16, 2019

Something must have changed in the way fixed-hash types, e.g. H160 derefs to &[u8], not sure what/why tbh but seems to require an explicit deref now.

@ordian
Copy link
Copy Markdown
Contributor

ordian commented May 16, 2019

It seems like we duplicate functionality here. In ethereum-types we use impl-rlp crate. Maybe we should remove ethereum feature altogether from rlp? And use impl-rlp in ethbloom?

@dvdplm
Copy link
Copy Markdown
Contributor Author

dvdplm commented May 17, 2019

It seems like we duplicate functionality here.

You mean it looks like impl-rlp does the same job as the ethereum feature in rlp?

@sorpaas knows better but I suspect he did it this way to keep the rlp crate directly usable from parity-ethereum? I'm not sure if parity-ethereum can switch to use impl-rlp, I haven't dug in but impl-rlp seems very minimal and parity-ethereum probably needs more functionality?

Either way: it is not directly relevant to this PR?

@ordian
Copy link
Copy Markdown
Contributor

ordian commented May 17, 2019 •
edited
Loading

I haven't dug in but impl-rlp seems very minimal and parity-ethereum probably needs more functionality?

ethereum feature of rlp does exactly what impl-rlp does in ethereum-types (+ it implements rlp for Bloom),
also see openethereum/parity-ethereum@9bcb992

@ordian
Copy link
Copy Markdown
Contributor

ordian commented May 17, 2019

Either way: it is not directly relevant to this PR?

it's relevant in a sense, that both ways are technically breaking changes, and I suggest we remove ethereum feature completely

@dvdplm
Copy link
Copy Markdown
Contributor Author

dvdplm commented May 17, 2019

I suggest we remove ethereum feature completely

Done. We need to wait for openethereum/parity-ethereum#10670 though to merge it.
Are aware of any other project using rlp with the ethereum feature? /cc @sorpaas

ordian
ordian previously approved these changes May 17, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@ordian ordian left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Do you want to bump (minor, hm not sure anymore) versions of dependent crates in a separate PR?

Comment thread primitive-types/impls/rlp/Cargo.toml Outdated
Comment thread triehash/Cargo.toml Outdated
dvdplm and others added 2 commits May 17, 2019 12:29
@dvdplm @ordian
Update primitive-types/impls/rlp/Cargo.toml
8262920 
Co-Authored-By: Andronik Ordian <write@reusable.software>
@dvdplm @ordian
Update triehash/Cargo.toml
88c6cf5 
Co-Authored-By: Andronik Ordian <write@reusable.software>
@dvdplm
Copy link
Copy Markdown
Contributor Author

dvdplm commented May 19, 2019

@ordian yes, I'm hazy on which crates/projects depend on rlp but I don't think there are many, other than impl-rlp?

@ordian
Copy link
Copy Markdown
Contributor

ordian commented May 20, 2019

@dvdplm I meant crates that you've touched in this PR, like primitive-types (also ethereum-types)

@dvdplm
Copy link
Copy Markdown
Contributor Author

dvdplm commented May 20, 2019

@dvdplm I meant crates that you've touched in this PR, like primitive-types (also ethereum-types)

Bumped impl-rlp and triehash.

ordian
ordian reviewed May 20, 2019
View reviewed changes
Comment thread primitive-types/impls/rlp/Cargo.toml Outdated
@ordian ordian dismissed their stale review May 20, 2019 13:35

Stale

sorpaas
sorpaas approved these changes May 20, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

LGTM!

@ordian ordian mentioned this pull request May 23, 2019
Merged
ordian
ordian reviewed May 23, 2019
View reviewed changes
Comment thread keccak-hash/Cargo.toml
@dvdplm
Merge branch 'master' into dp/chore/update-rlp-to-ethereum-types-0.5
1cc5e43 
* master:
  Remove unused test support files (#162)
  [parity-crypto] bump version to 0.4.0 (#149)
  [parity-crypto] zero memory for hmac signing keys (#157)
  Added absolute path for 'Result' usage in macros to avoid type collisions. (#160)
@ordian ordian mentioned this pull request May 23, 2019
Closed
@dvdplm dvdplm merged commit 1cc734a into master May 23, 2019
@ordian ordian deleted the dp/chore/update-rlp-to-ethereum-types-0.5 branch May 23, 2019 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@ascjones ascjones ascjones approved these changes

@ordian ordian ordian approved these changes

@sorpaas sorpaas sorpaas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dvdplm @ordian @ascjones @sorpaas