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.

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

feat(runtime): add metadata signed-extension #2097

Merged
merged 1 commit into from
Jul 30, 2024
Merged

feat(runtime): add metadata signed-extension #2097

merged 1 commit into from
Jul 30, 2024

Conversation

enddynayn
Copy link
Collaborator

@enddynayn enddynayn commented Jul 24, 2024 •
edited
Loading

Add a new signed extension that enables the metadata hash verification feature approved under RFC 0078. This enhancement will support the new generic ledger hardware wallet app and future hardware wallets within the Polkadot ecosystem.

Reference implementation

@enddynayn enddynayn force-pushed the feat/add-metadata-hash-polkadot-br-dep branch 2 times, most recently from cccf0c3 to e2b6389 Compare July 24, 2024 17:52
@codecov Codecov
Copy link

codecov bot commented Jul 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented and removed metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels Jul 24, 2024
@enddynayn enddynayn force-pushed the feat/add-metadata-hash-polkadot-br-dep branch 4 times, most recently from 975b993 to 4064783 Compare July 26, 2024 20:50
@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Jul 26, 2024
@enddynayn enddynayn marked this pull request as ready for review July 26, 2024 21:36
@enddynayn enddynayn requested a review from wilwade as a code owner July 26, 2024 21:36
@enddynayn enddynayn marked this pull request as draft July 26, 2024 21:36
@enddynayn enddynayn removed the request for review from wilwade July 26, 2024 21:37
@enddynayn enddynayn marked this pull request as ready for review July 29, 2024 14:57
saraswatpuneet
saraswatpuneet reviewed Jul 29, 2024
View reviewed changes
runtime/frequency/src/lib.rs
@@ -304,6 +304,7 @@ pub type SignedExtra = (
pallet_frequency_tx_payment::ChargeFrqTransactionPayment<Runtime>,
pallet_msa::CheckFreeExtrinsicUse<Runtime>,
pallet_handles::handles_signed_extension::HandlesSignedExtension<Runtime>,
frame_metadata_hash_extension::CheckMetadataHash<Runtime>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we do this for passkey unsigned ext? Maybe yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I think that sounds like a good idea.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is to ensure that the offline wallets are signing the expected data structure (metadata). Since we are doing the signing via passkey adding this does not bring any additional value at this point since the metadata is a totally foreign concept from passkey's perspective.

We might be able to utilize this check in future by adding this metadata hash inside PasskeyCall<T: Config> type and that would allow us to ensure the passkey signature was done on a correct metadata.

Copy link
Collaborator Author

@enddynayn enddynayn Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I’ve been considering it. Since this is an unsigned transaction, it might not be the best fit. However, I believe we are still signing the call function within the outer call. I wonder if we can verify that the hash of the metadata used at the time of signing matches the root.

We would have to implement the UI for this in any case.

saraswatpuneet
saraswatpuneet approved these changes Jul 29, 2024
View reviewed changes
Copy link
Collaborator

@saraswatpuneet saraswatpuneet left a comment

Choose a reason for hiding this comment

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

Lgtm , maybe we can add a story for passkey pallet to also add/support hash verification

claireolmstead
claireolmstead approved these changes Jul 29, 2024
View reviewed changes
Copy link
Collaborator

@claireolmstead claireolmstead left a comment

Choose a reason for hiding this comment

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

Looks great!

aramikm
aramikm approved these changes Jul 29, 2024
View reviewed changes
Copy link
Collaborator

@aramikm aramikm left a comment

Choose a reason for hiding this comment

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

Looks good overall! Added some questions

runtime/frequency/build.rs
.export_heap_base()
.import_memory()
substrate_wasm_builder::WasmBuilder::init_with_defaults()
.enable_metadata_hash("FRQCY", 8)
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: why is this 8 for decimals? On reference implementation it's 14

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for asking about the number of decimals for the token. It prompted me to check, and I confirmed that both Frequency and Paseo have the same number of decimals.

runtime/frequency/src/lib.rs
@@ -304,6 +304,7 @@ pub type SignedExtra = (
pallet_frequency_tx_payment::ChargeFrqTransactionPayment<Runtime>,
pallet_msa::CheckFreeExtrinsicUse<Runtime>,
pallet_handles::handles_signed_extension::HandlesSignedExtension<Runtime>,
frame_metadata_hash_extension::CheckMetadataHash<Runtime>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is to ensure that the offline wallets are signing the expected data structure (metadata). Since we are doing the signing via passkey adding this does not bring any additional value at this point since the metadata is a totally foreign concept from passkey's perspective.

We might be able to utilize this check in future by adding this metadata hash inside PasskeyCall<T: Config> type and that would allow us to ensure the passkey signature was done on a correct metadata.

@enddynayn enddynayn force-pushed the feat/add-metadata-hash-polkadot-br-dep branch from 3edcc5f to 7148439 Compare July 29, 2024 21:34
@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Jul 29, 2024
@enddynayn enddynayn force-pushed the feat/add-metadata-hash-polkadot-br-dep branch from 7148439 to 5cf3517 Compare July 29, 2024 21:38
@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Jul 29, 2024
@enddynayn enddynayn force-pushed the feat/add-metadata-hash-polkadot-br-dep branch from 5cf3517 to 687dc11 Compare July 30, 2024 13:09
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release and removed metadata-changed Metadata has changed since the latest full release labels Jul 30, 2024
@enddynayn
feat(runtime): add metadata signed-extension
4c6082b 
Add a new signed extension that enables the metadata hash verification feature approved under [RFC 0078](https://polkadot-fellows.github.io/RFCs/approved/0078-merkleized-metadata.html#rfc-0078-merkleized-metadata). This enhancement will support the new generic ledger hardware wallet app and future hardware wallets within the Polkadot ecosystem.

[Reference implementation](paritytech/polkadot-sdk#4580)
@enddynayn enddynayn force-pushed the feat/add-metadata-hash-polkadot-br-dep branch from 9d93940 to 4c6082b Compare July 30, 2024 16:17
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release and removed metadata-changed Metadata has changed since the latest full release labels Jul 30, 2024
@enddynayn enddynayn merged commit cbf4c85 into main Jul 30, 2024
27 checks passed
@enddynayn enddynayn deleted the feat/add-metadata-hash-polkadot-br-dep branch July 30, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aramikm aramikm aramikm approved these changes

@claireolmstead claireolmstead claireolmstead approved these changes

@saraswatpuneet saraswatpuneet saraswatpuneet approved these changes

@JoeCap08055 JoeCap08055 JoeCap08055 approved these changes

Assignees
No one assigned
Labels
metadata-changed Metadata has changed since the latest full release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@enddynayn @aramikm @claireolmstead @saraswatpuneet @JoeCap08055