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

fix: MSA owner should be able to invoke non-signature methods on SignatureRequired schemas directly #1844

Conversation

JoeCap08055
Copy link
Collaborator

@JoeCap08055 JoeCap08055 commented Jan 19, 2024

Goal

The goal of this PR is to make it possible for MSA owners to call non-signature stateful-storage methods directly without requiring a separate payload signature.

Adds e2e test scenarios to validate this correct behavior for both signature and non-signature stateful-storage extrinsics

Closes #1793

Discussion

Testing

  • Run e2e tests for the stateful-storage pallet against an unmodified frequency chain; should result in ~ 6 errors
  • Run e2e tests against a locally-built chain from this branch; errors should be resolved

TODO

  • Add Rust unit tests

Checklist

  • Chain spec updated
  • Custom RPC OR Runtime API added/changed? Updated js/api-augment.
  • Design doc(s) updated
  • Tests added
  • Benchmarks added
  • Weights updated

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (70aec76) 82.90% compared to head (96702d5) 82.95%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1844      +/-   ##
==========================================
+ Coverage   82.90%   82.95%   +0.04%     
==========================================
  Files          56       56              
  Lines        4540     4552      +12     
==========================================
+ Hits         3764     3776      +12     
  Misses        776      776              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@wilwade wilwade left a comment

Choose a reason for hiding this comment

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

These aren't the signature require extrinsics, but I think all of your tests should pass without any changes to the lib.rs due to https://github.com/LibertyDSNP/frequency/blob/c92466510fb78a4885a8977d1d7444916ff77465/pallets/stateful-storage/src/lib.rs#L762-L763

@JoeCap08055
Copy link
Collaborator Author

These aren't the signature require extrinsics, but I think all of your tests should pass without any changes to the lib.rs due to

https://github.com/LibertyDSNP/frequency/blob/c92466510fb78a4885a8977d1d7444916ff77465/pallets/stateful-storage/src/lib.rs#L762-L763

That's right; these aren't the signature required extrinsics. That's not the point of the PR; in fact, I pointed out in the original issue here that the problem seems to have been mis-stated. The signature-required extrinsics always worked correctly with a signature on a SignatureRequired schema, but I believe the non-signature extrinsics should also work for said schemas if invoked by the schema owner.

I verified that against the current Frequency release, my new e2e tests fail; with these modifications, they pass.

@wilwade
Copy link
Collaborator

wilwade commented Jan 22, 2024

That's right; these aren't the signature required extrinsics. That's not the point of the PR; in fact, I pointed out in the original issue here that the problem seems to have been mis-stated. The signature-required extrinsics always worked correctly with a signature on a SignatureRequired schema, but I believe the non-signature extrinsics should also work for said schemas if invoked by the schema owner.

Ah I see now. Sorry about that. I missed connecting this with the other. Calling the normal one instead of the with_signature is a nice way around this issue.

Will re-review.

Copy link
Collaborator

@wilwade wilwade left a comment

Choose a reason for hiding this comment

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

Now that I understand what is going on, this is a great fix! A few notes, but my guess is that those were mostly already on your todo list for before it gets out of draft mode

- Update documentation for appy_item_actions, delete_page, upsert_page
- Add unit tests to cover calling above extrinsics w/both owner & non-owner (delegate)
@JoeCap08055 JoeCap08055 marked this pull request as ready for review January 22, 2024 17:07
@JoeCap08055 JoeCap08055 requested a review from demisx as a code owner January 22, 2024 17:07
Copy link
Collaborator

@wilwade wilwade left a comment

Choose a reason for hiding this comment

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

I haven't run the code locally, but this captures a nice simple solution to the problem with minimal code changes. (and a lot of new tests)

  • Read code
  • Validated it solves the original issue

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, added some nits. Spec version needs to get updated!

  • Ran the local chain
  • Ran unit & e2e tests

Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

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

I only reviewed the code, had read the related issue + discussion fairly well. Looks like an elegant solution to me. One naming suggestion but NBD.

JoeCap08055 and others added 2 commits January 23, 2024 11:39
Copy link
Collaborator

@enddynayn enddynayn left a comment

Choose a reason for hiding this comment

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

wonderful! 👍

@JoeCap08055 JoeCap08055 enabled auto-merge (squash) January 23, 2024 17:53
@JoeCap08055 JoeCap08055 disabled auto-merge January 23, 2024 17:53
…n-all-extrinsics-on-stateful-pallet-if-called-from-owner
@JoeCap08055 JoeCap08055 enabled auto-merge (squash) January 23, 2024 18:03
@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Jan 23, 2024
@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 Jan 23, 2024
@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 Jan 24, 2024
@JoeCap08055 JoeCap08055 merged commit 8679b88 into main Jan 24, 2024
30 checks passed
@JoeCap08055 JoeCap08055 deleted the 1793-requiredsignature-schemas-should-work-on-all-extrinsics-on-stateful-pallet-if-called-from-owner branch January 24, 2024 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metadata-changed Metadata has changed since the latest full release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RequiredSignature schemas should work on all extrinsics on stateful-pallet if called from owner
6 participants