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

Add Validate impl for ScMap and Map ScVal variant that can be invalid #112

Merged
merged 75 commits into from
Jul 21, 2022

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Jul 19, 2022

What

Add Validate trait and impl for ScMap and ScVal::Map variant that can be invalid regardless of any other context.

Why

Facilitate users of the XDR validating that the ScVal types are valid. There are likely to be multiple places that we'll need to validate these XDR values. For map types this is especially easy to mess up, and will be one thing in the toolbox that we use to prevent footguns so that users do not see signature failures for transactions that they submit a misordered map for that they signed for.

Follow up to #111
Close #113 #98

Known limitations

[TODO or N/A]

Base automatically changed from issue106 to main July 19, 2022 05:06
@leighmcculloch leighmcculloch linked an issue Jul 19, 2022 that may be closed by this pull request
@graydon
Copy link
Contributor

graydon commented Jul 20, 2022

@leighmcculloch I've pushed a little more here to cover the "construct a sorted map" scenario and tidied the comment. Hope it covers what you wanted! Let me know.

Copy link
Member Author

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

✅ Thanks @graydon !

src/scmap.rs Show resolved Hide resolved
src/scmap.rs Outdated Show resolved Hide resolved
@leighmcculloch leighmcculloch marked this pull request as ready for review July 20, 2022 02:13
src/scmap.rs Outdated

pub fn sorted_from_pairs<K, V, I: Iterator<Item = (K, V)>>(pairs: I) -> Result<ScMap, ()>
where
ScVal: From<K> + From<V>,
Copy link
Member Author

Choose a reason for hiding this comment

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

It might create more flexibility to make this use the Into conversion, or better yet, the TryInto conversion. Using Into instead of From will make it work for types that implement only the Into conversion. Using the Try version will make it work with types that don't offer an errorless conversion, like Vec.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

@leighmcculloch leighmcculloch enabled auto-merge (squash) July 21, 2022 00:16
@leighmcculloch
Copy link
Member Author

Thanks @graydon. I made some more tweaks to handle keys and values that are TryInto<ScVal> rather than Into<ScVal>. I realized after we conversed on here earlier that some types don't have infallible conversions to ScVal and would be excluded.

Comment on lines +25 to +32
pub fn sorted_from_pairs<K, V, I>(pairs: I) -> Result<ScMap, Error>
where
K: TryInto<ScVal>,
V: TryInto<ScVal>,
I: Iterator<Item = (K, V)>,
{
Self::sorted_from_entries(pairs)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This fn is now technically redundant because the same values can be passed directly to the entries fn. But this fn has a clearer signature that is easier to understand, so I think it is still useful so I kept it.

todo!()
// Check the map is sorted by key, and there are no keys that are
// duplicates.
if self.windows(2).all(|w| w[0].key < w[1].key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this .windows function? Can't find its definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah cool!

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.

Test Fwk: Add ScVal validation trait and fns (ScMap)
3 participants