-
Notifications
You must be signed in to change notification settings - Fork 24
Feat/intents extrinsics and rpcs #2570
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
Feat/intents extrinsics and rpcs #2570
Conversation
# Goal The goal of this PR is to implement *only* the *types* and *pallet storage* for the following new on-chain entities: * Intents * IntentGroups Closes #2561 **NOTE:** No tests or benchmarks added in this PR, as it only contains types and storage, no executable runtime code (coming in a separate PR) # Checklist - [x] Updated Pallet Readme? - [ ] Updated js/api-augment for Custom RPC APIs? - [x] Design doc(s) updated? - [ ] Unit Tests added? - [ ] e2e Tests added? - [ ] Benchmarks added? - [ ] Spec version incremented? # Conflicts: # pallets/schemas/src/lib.rs # pallets/schemas/src/tests/other_tests.rs # pallets/schemas/src/types.rs # runtime/frequency/src/lib.rs
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
aramikm
left a comment
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.
Overall looks good.
Approving to unblock but there are some comments that needs attention before merging
saraswatpuneet
left a comment
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.
looks good, one question around update path
7b6fdcf
into
feat/schemas-permissions-development
| pub intent_group_id: u16, | ||
| /// The list of currently supported IntentIds for this IntentGroup | ||
| pub intent_ids: Option<Vec<IntentId>>, | ||
| pub intent_ids: Vec<IntentId>, |
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.
Was this never meant to be an option?
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.
Not sure what this will eventually look like... an upcoming PR in the epic will revisit these runtime APIs and responses, anyway
| } | ||
|
|
||
| /// Adds a given schema to storage. The schema in question must be of length | ||
| /// Adds a given schema to storage. (testnet) |
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.
Following the existing pattern in the pallet, it exists but the call is filtered in the Runtime on Mainnet. Not sure why it's done that way vs a feature flag; that decision predates me.
| // check that doesn't start with a decimal digit. | ||
| // (This also handles the case where the value is all numeric, because it would also | ||
| // start with a decimal digit.) | ||
| ensure!(namespace[0].is_ascii_alphabetic(), Error::<T>::InvalidSchemaNameCharacters); |
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.
nit: the comment isn't precisely true, otherwise you'd just check for !~[0-9]
Goal
The goal of this PR is to implement the extrinsics, RPCs and unit tests for Intents and IntentGroups.
Closes #2564
Discussion
Checklist