Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Offchain signing followup #1017

Merged
merged 3 commits into from
Apr 22, 2020
Merged

Offchain signing followup #1017

merged 3 commits into from
Apr 22, 2020

Conversation

rakanalh
Copy link
Contributor

This is a follow up PR for #985 where some feedback was provided after the PR was merged.

@rakanalh rakanalh added the A0-please_review Pull request needs code review. label Apr 21, 2020
bkchr
bkchr reviewed Apr 21, 2020
View reviewed changes
runtime/common/src/parachains.rs Outdated Show resolved Hide resolved
runtime/common/src/parachains.rs
pub struct FishermanAuthorityId;
impl system::offchain::AppCrypto<<Signature as Verify>::Signer, Signature> for FishermanAuthorityId {
type RuntimeAppPublic = FishermanId;
type GenericSignature = sr25519::Signature;
Copy link
Member

Choose a reason for hiding this comment

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

This is really over-complicated code...

Copy link
Contributor

Choose a reason for hiding this comment

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

Noted here: paritytech/substrate#5662 Unfortunately using existing traits (IsWrappedBy) is non trivial and it is still to be addressed in substrate. But yeah, this code is definitely redundant.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/paritytech/substrate/blob/999f4832e715f20a17fc2a3060bbe2ec88e7dbff/primitives/application-crypto/src/lib.rs#L204 this is even much easier.

tomusdrw
tomusdrw approved these changes Apr 22, 2020
View reviewed changes
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm!

@tomusdrw tomusdrw added A5-grumble and removed A0-please_review Pull request needs code review. labels Apr 22, 2020
@rakanalh rakanalh added A0-please_review Pull request needs code review. and removed A5-grumble labels Apr 22, 2020
@rakanalh rakanalh requested review from bkchr and tomusdrw April 22, 2020 11:17
bkchr
bkchr approved these changes Apr 22, 2020
View reviewed changes
runtime/common/src/parachains.rs
pub struct FishermanAuthorityId;
impl system::offchain::AppCrypto<<Signature as Verify>::Signer, Signature> for FishermanAuthorityId {
type RuntimeAppPublic = FishermanId;
type GenericSignature = sr25519::Signature;
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/paritytech/substrate/blob/999f4832e715f20a17fc2a3060bbe2ec88e7dbff/primitives/application-crypto/src/lib.rs#L204 this is even much easier.

@bkchr bkchr merged commit f8ac46d into paritytech:master Apr 22, 2020
@rakanalh rakanalh deleted the offchain-signing-followup branch April 22, 2020 11:46
@tomusdrw
Copy link
Contributor

tomusdrw commented Apr 22, 2020 •
edited

https://github.com/paritytech/substrate/blob/999f4832e715f20a17fc2a3060bbe2ec88e7dbff/primitives/application-crypto/src/lib.rs#L204 this is even much easier.

But this is AppKey, which I believe is not supposed to be used in the runtime. If it is then we should remove RuntimeAppPublic cause now the only thing we have is confusion.

Seems AppKey is supposed to just describe the types for all keys, so it definitely is worth looking into.

@rphmeier
Copy link
Contributor

rphmeier commented Apr 24, 2020 •
edited

Sorry to review after merge again, but I still prefer to have this key fully defined in the primitives crate. I don't know what it's doing in runtime/parachains - is it because of the system::offchain trait impl and the orphan rules?

It seems much better in that case to make the parachain code generic over the key type and define this instead at the level of polkadot-runtime, kusama-runtime, and test-runtime crates.

svyatonik added a commit that referenced this pull request Jun 25, 2021
@svyatonik
Squashed 'bridges/' changes from b2099c5..23dda62
a42dcbd 
23dda62 Rococo <> Wococo messages relay (#1030)
bcde21d Update the wasm builder to substrate master (#1029)
a8318ce Make target signer optional when sending message. (#1018)
f8602e1 Fix insufficient balance when send message. (#1020)
d95c0a7 greedy relayer don't need message dispatch to be prepaid if dispatch is supposed to be paid at the target chain (#1016)
ad5876f Update types. (#1027)
116cbbc CI: fix starting the pipeline (#1022)
7e0fadd Add temporary `canary` job (#1019)
6787091 Update types to contain dispatch_fee_payment (#1017)
03f79ad Allow Root to assume SourceAccount. (#1011)
372d019 Return dispatch_fee_payment from message details RPC (#1014)
604eb1c Relay basic single-bit message dispatch results back to the source chain (#935)
bf52fff Use plain source_queue view when selecting nonces for delivery (#1010)
fc5cf7d pay dispatch fee at target chain (#911)
1e35477 Bump Substrate to `286d7ce` (#1006)
7ad07b3 Add --only-mandatory-headers mode (#1004)
5351dc9 Messages relayer operating mode (#995)
9bc29a7 Rococo <> Wococo relayer balance guard (#998)
bc17341 rename messages_dispatch_weight -> message_details (#996)
95be244 Bump Rococo and Wococo spec versions (#999)
c35567b Move ChainWithBalances::NativeBalance -> Chain::Balance (#990)
1bfece1 Fix some nits (#988)
334ea0f Increase pause before starting relays again (#989)
7fb8248 Fix clippy in test code (#993)
d60ae50 fix clippy issues (#991)
75ca813 Make sure GRANDPA shares state with RPC. (#987)
da2a38a Bump Substrate (#986)
5a9862f Update submit finality proof weight formula (#981)
69df513 Flag for rejecting all outbound messages (#982)
14d0506 Add script to setup bench machine. (#984)
e74e8ab Move CI from GitHub Actions to GitLab (#814)
c5ca5dd Custom justification verification (#979)
643f10d Always run on-demand headers relay in complex relay (#975)
a35b0ef Add JSON type definitions for Rococo<>Wococo bridge (#977)
0eb83f2 Update cargo.deny (#980)
e1d1f4c Bump Rococo/Wococo spec_version (#976)
deac90d increase pause before starting relays (#974)
68d6d79 Revert to use InspectCmd, bump substrate `6bef4f4` (#966)
66e1508 Avoid hashing headers twice in verify_justification (#973)
a31844f Bump `environmental` dependency (#972)
2a4c29a in auto-relays keep trying to connect to nodes until connection is established (#971)
0e767b3 removed stray file (#969)
b9545dc Serve multiple lanes with single complex relay instance (#964)
73419f4 Correct type error (#968)
bac256f Start finality relay spec-version guards for Rococo <> Wococo finality relays (#965)
bfd7037 pass source and target chain ids to account_ownership_proof (#963)
8436073 Upstream changes from Polkadot repo (#961)
e58d851 Increase account endowment amount (#960)

git-subtree-dir: bridges
git-subtree-split: 23dda6248236b27f20d76cbedc30e189cc6f736c
ghost pushed a commit that referenced this pull request Jun 25, 2021
@svyatonik
Squashed 'bridges/' changes from b2099c5..23dda62 (#3369)
944ff40 
23dda62 Rococo <> Wococo messages relay (#1030)
bcde21d Update the wasm builder to substrate master (#1029)
a8318ce Make target signer optional when sending message. (#1018)
f8602e1 Fix insufficient balance when send message. (#1020)
d95c0a7 greedy relayer don't need message dispatch to be prepaid if dispatch is supposed to be paid at the target chain (#1016)
ad5876f Update types. (#1027)
116cbbc CI: fix starting the pipeline (#1022)
7e0fadd Add temporary `canary` job (#1019)
6787091 Update types to contain dispatch_fee_payment (#1017)
03f79ad Allow Root to assume SourceAccount. (#1011)
372d019 Return dispatch_fee_payment from message details RPC (#1014)
604eb1c Relay basic single-bit message dispatch results back to the source chain (#935)
bf52fff Use plain source_queue view when selecting nonces for delivery (#1010)
fc5cf7d pay dispatch fee at target chain (#911)
1e35477 Bump Substrate to `286d7ce` (#1006)
7ad07b3 Add --only-mandatory-headers mode (#1004)
5351dc9 Messages relayer operating mode (#995)
9bc29a7 Rococo <> Wococo relayer balance guard (#998)
bc17341 rename messages_dispatch_weight -> message_details (#996)
95be244 Bump Rococo and Wococo spec versions (#999)
c35567b Move ChainWithBalances::NativeBalance -> Chain::Balance (#990)
1bfece1 Fix some nits (#988)
334ea0f Increase pause before starting relays again (#989)
7fb8248 Fix clippy in test code (#993)
d60ae50 fix clippy issues (#991)
75ca813 Make sure GRANDPA shares state with RPC. (#987)
da2a38a Bump Substrate (#986)
5a9862f Update submit finality proof weight formula (#981)
69df513 Flag for rejecting all outbound messages (#982)
14d0506 Add script to setup bench machine. (#984)
e74e8ab Move CI from GitHub Actions to GitLab (#814)
c5ca5dd Custom justification verification (#979)
643f10d Always run on-demand headers relay in complex relay (#975)
a35b0ef Add JSON type definitions for Rococo<>Wococo bridge (#977)
0eb83f2 Update cargo.deny (#980)
e1d1f4c Bump Rococo/Wococo spec_version (#976)
deac90d increase pause before starting relays (#974)
68d6d79 Revert to use InspectCmd, bump substrate `6bef4f4` (#966)
66e1508 Avoid hashing headers twice in verify_justification (#973)
a31844f Bump `environmental` dependency (#972)
2a4c29a in auto-relays keep trying to connect to nodes until connection is established (#971)
0e767b3 removed stray file (#969)
b9545dc Serve multiple lanes with single complex relay instance (#964)
73419f4 Correct type error (#968)
bac256f Start finality relay spec-version guards for Rococo <> Wococo finality relays (#965)
bfd7037 pass source and target chain ids to account_ownership_proof (#963)
8436073 Upstream changes from Polkadot repo (#961)
e58d851 Increase account endowment amount (#960)

git-subtree-dir: bridges
git-subtree-split: 23dda6248236b27f20d76cbedc30e189cc6f736c
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bkchr bkchr bkchr approved these changes

@rphmeier rphmeier Awaiting requested review from rphmeier

@tomusdrw tomusdrw Awaiting requested review from tomusdrw

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rakanalh @tomusdrw @rphmeier @bkchr @paritytech-ci