Skip to content
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

Introduce baseline types required to implement validator set updates #273

Merged
merged 49 commits into from
Aug 10, 2022

Conversation

sug0
Copy link
Collaborator

@sug0 sug0 commented Aug 3, 2022

Implements the second item of issue #242

@sug0 sug0 marked this pull request as ready for review August 8, 2022 13:06
@sug0 sug0 requested review from james-chf and batconjurer August 8, 2022 13:06
@sug0
Copy link
Collaborator Author

sug0 commented Aug 8, 2022

I haven't added unit tests to these yet. I might leave that to the next PR (PrepareProposal), since I'll write the validation code then. idk, what do you think?

@james-chf
Copy link
Contributor

james-chf commented Aug 9, 2022

There is one more Cargo.lock - make -C wasm_for_tests/wasm_source check - not currently compiling* for me, though I think there might be something odd with my set up as this type definitely exists

error[E0432]: unresolved import `crate::types::transaction::protocol::ProtocolTxType`
 --> /opt/workspace/heliax/repos/anoma-review/shared/src/types/vote_extensions.rs:7:5
  |
7 | use crate::types::transaction::protocol::ProtocolTxType;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `ProtocolTxType` in `types::transaction::protocol`

For more information about this error, try `rustc --explain E0432`.

shared/src/proto/types.rs Outdated Show resolved Hide resolved
@sug0 sug0 requested a review from james-chf August 9, 2022 10:03
shared/src/proto/types.rs Outdated Show resolved Hide resolved
shared/src/types/vote_extensions/validator_set_update.rs Outdated Show resolved Hide resolved
shared/src/types/vote_extensions/validator_set_update.rs Outdated Show resolved Hide resolved
shared/src/types/vote_extensions/validator_set_update.rs Outdated Show resolved Hide resolved
shared/src/types/vote_extensions/validator_set_update.rs Outdated Show resolved Hide resolved
@sug0 sug0 marked this pull request as draft August 9, 2022 12:55
@sug0 sug0 requested review from batconjurer and james-chf August 9, 2022 14:10
@sug0 sug0 marked this pull request as ready for review August 9, 2022 14:12
Copy link
Member

@batconjurer batconjurer left a comment

Choose a reason for hiding this comment

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

LGTM!

#[derive(
Clone, Debug, BorshSerialize, BorshDeserialize, Serialize, Deserialize,
)]
pub struct Signed<T: BorshSerialize + BorshDeserialize> {
pub struct Signed<T, S = SerializeWithBorsh> {
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good if we could bound S here (if it's possible?) so that it's impossible to construct a Signed with an invalid tag, something like

pub struct Signed<T, S: SignedSerialize<T> = SerializeWithBorsh>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we already tighten Signed at the impl block starting on line 148. you can only verify signatures of Signed instances with a tag which implements SignedSerialize. you can't use the struct itself to instantiate new values, since it has a private field, but you can use Signed::new_from instead. regardless, we hit the same wall, this instance can't do jack if S does not implement SignedSerialize.

tl;dr I don't think this is an issue

@sug0 sug0 merged commit 21b9136 into eth-bridge-integration Aug 10, 2022
@sug0 sug0 deleted the tiago/ethbridge/valset-update-types branch August 10, 2022 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants