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

Enforce key format in keyset creation #918

Merged
merged 4 commits into from
Nov 2, 2021
Merged

Conversation

sirlensalot
Copy link
Contributor

No description provided.

keyFormats = [ed25519Hex]

enforceKeyFormats :: HasInfo i => i -> KeySet -> Eval e ()
enforceKeyFormats i (KeySet ks _p) = foldM go () ks
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enforceKeyFormats i (KeySet ks _p) = foldM go () ks
enforceKeyFormats i (KeySet ks _p) = traverse_ go ks
where
go k = unless (any ($ k) keyFormats) $ evalError' i "Invalid keyset"

traverse_ from Data.Foldable doesn't need Traversable.

@sirlensalot sirlensalot merged commit 4b23e71 into master Nov 2, 2021
@sirlensalot sirlensalot deleted the feat/enforce-ks-formats branch November 2, 2021 03:42
sirlensalot added a commit that referenced this pull request Nov 2, 2021
* Enforce key format in keyset creation

* more tests

* review golf

* only support lowercase hex

Co-authored-by: Stuart Popejoy <[email protected]>

-- | Supported key formats.
keyFormats :: [KeyFormat]
keyFormats = [ed25519Hex]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these formats be scheme-aware?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Schemes are not wallet-supported atm and with this change will need upgrades for support; they also need to be self-identifying so that e.g. an ETH sig will be smth like eth:0xadf5adf4a... at which point this mechanism will naturally support adding that.

(env-exec-config ["EnforceKeyFormats"])
(env-data
{ 'bad: ['foo]
, 'short: ["12440d374865bdf0a3349634a70d1317fc279e7e13db98f2199ac5e7378975"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add one character to this short test to cover the largest possible number of failure cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR has merged but I manually checked and that case works.

(env-data
{ 'bad: ['foo]
, 'short: ["12440d374865bdf0a3349634a70d1317fc279e7e13db98f2199ac5e7378975"]
, 'long: ["12440d374865bdf0a3349634a70d1317fc279e7e13db98f2199ac5e7378975eaea"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto for shortening this long one by one character. Since we're operating in the string domain it's characters, not after-decoded bytes, that are the fundamental unit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Manually checked.

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.

4 participants