-
Notifications
You must be signed in to change notification settings - Fork 30
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
Conversation
@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. |
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.
✅ Thanks @graydon !
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>, |
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.
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.
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.
Done.
Thanks @graydon. I made some more tweaks to handle keys and values that are |
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) | ||
} |
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.
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) { |
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.
What is this .windows
function? Can't find its definition.
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.
The function is https://doc.rust-lang.org/std/primitive.slice.html#method.windows.
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.
Ah cool!
What
Add
Validate
trait and impl forScMap
andScVal::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]