-
Notifications
You must be signed in to change notification settings - Fork 214
chore: update ethereum-types version #3247
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.
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
base: master
Are you sure you want to change the base?
Conversation
|
retest this please |
tests/conflux/messages.py
Outdated
| return False | ||
| elif serial == b'\x01': | ||
| return True | ||
| elif serial == b'': |
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 see the RLP change here : https://github.com/paritytech/parity-common/pull/572
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.
Done
|
retest this please |
2 similar comments
|
retest this please |
|
retest this please |
| def test_get_tx_by_hash_errors(client: RpcClient): | ||
|
|
||
| assert_raises_rpc_error( | ||
| -32602, |
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.
Remove unnecessary error tests.
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.
Done
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 "dir" crate also have a "ethereum-types" dependency, recommend update it too
- The rlp behaviour change seems is broken, seems this pr need a resync the data from genesis. we need figure a way to merge it
Reviewed 45 of 51 files at r1, 7 of 9 files at r2, 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions
tests/conflux/messages.py line 65 at r3 (raw file):
) # Copied from rlp.sedes.Boolean, but encode False to 0x00, not empty.
same here
integration_tests/conflux/messages.py line 65 at r3 (raw file):
) # Copied from rlp.sedes.Boolean, but encode False to 0x00, not empty.
update the comment here
|
retest this please |
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.
Reviewed 18 of 18 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @iosh)
| def test_get_tx_by_hash_errors(client: RpcClient): | ||
|
|
||
| assert_raises_rpc_error( | ||
| -32602, |
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.
Done
tests/conflux/messages.py
Outdated
| return False | ||
| elif serial == b'\x01': | ||
| return True | ||
| elif serial == b'': |
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.
Done
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.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @iosh)
|
@peilun-conflux @ChenxingLi Please also help review this PR |
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.
Revert all changes related to rlp, put it in a separate PR. Read the issue #3254 for the details related to rlp.
Reviewed 22 of 51 files at r1, 7 of 18 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @peilun-conflux)
|
retest this please |
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.
Reviewed 12 of 25 files at r5, 5 of 9 files at r6, 1 of 13 files at r7, 5 of 11 files at r9, 11 of 11 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @peilun-conflux)
tools/consensus_bench/Cargo.toml line 20 at r10 (raw file):
[patch.crates-io] rlp = { git = "https://github.com/Conflux-Chain/parity-common-rocksdb.git", rev = "b28832745c952c8bcac88348276b283c26de776a", package = "rlp" }
features is required
tools/evm-spec-tester/Cargo.toml line 41 at r10 (raw file):
[patch.crates-io] rlp = { git = "https://github.com/Conflux-Chain/parity-common-rocksdb.git", rev = "b28832745c952c8bcac88348276b283c26de776a", package = "rlp" }
features is required
Cargo.toml line 428 at r10 (raw file):
# parity crates # The fork of the rlp crate adds a legacy v0.4.x boolean encoding to the v0.6.x rlp = { git = "https://github.com/Conflux-Chain/parity-common-rocksdb.git", rev = "b28832745c952c8bcac88348276b283c26de776a", package = "rlp", features = [
since you use the patch feature, no need update it here, you can move the comment to patch part
Cargo.toml line 461 at r10 (raw file):
[patch.crates-io] sqlite3-sys = { git = "https://github.com/Conflux-Chain/sqlite3-sys.git", rev = "1de8e5998f7c2d919336660b8ef4e8f52ac43844" } rlp = { git = "https://github.com/Conflux-Chain/parity-common-rocksdb.git", rev = "b28832745c952c8bcac88348276b283c26de776a", package = "rlp" }
features is required here
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.
Reviewable status: 70 of 71 files reviewed, 4 unresolved discussions (waiting on @Pana and @peilun-conflux)
Cargo.toml line 428 at r10 (raw file):
Previously, Pana (Pana) wrote…
since you use the patch feature, no need update it here, you can move the comment to patch part
The patch does not allow use feature.
Cargo.toml line 461 at r10 (raw file):
Previously, Pana (Pana) wrote…
features is required here
Same here.
tools/consensus_bench/Cargo.toml line 20 at r10 (raw file):
Previously, Pana (Pana) wrote…
features is required
Same here.
tools/evm-spec-tester/Cargo.toml line 41 at r10 (raw file):
Previously, Pana (Pana) wrote…
features is required
Same here.
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.
Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @peilun-conflux)
|
retest this please |
|
Add two new custom types, CompatibleBool and LegacyBool, with custom encoding and decoding implementations. LegacyBool adheres to the decoding rules of RLP versions prior to 0.5, while CompatibleBool follows the rules for RLP version 0.5 and above, accepting 0x00 as false during decoding. |
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.
Reviewed 57 of 57 files at r12, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ChenxingLi and @peilun-conflux)
Package Upgrades:
keccak-hash:0.5->0.11.0rlp:0.4->0.6.1(Use forked version)ethereum-types:0.9->0.15.1** Changes **
parity-common/uint CHANGELOG#0.10.0
to_big_endianhas been renamed towrite_as_big_endian.to_little_endianhas been renamed towrite_as_little_endian.drain()has been replaced withas_raw().as_slice()has been replaced withas_ref().sig.r().into()has been updated toU256::from_big_endian(sig.r())for explicit construction from big-endian bytes.push_msg_id_leb128_encodingfromVec<u8>toBytesMut.This change is
resolve #3239
BREAKING CHANGE:The upgrade to rlp 0.6.1 :Previous behavior:True was often encoded as RLP 0x01.False was often encoded as RLP 0x00.New behavior:True is encoded as RLP 0x01.False is encoded as RLP 0x80 (representing an empty byte string).You can see the RLP change here :https://github.com/paritytech/parity-common/pull/572