Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

parity: fix UserDefaults json parser#9130

Closed
andresilva wants to merge 6 commits into
masterfrom
andre/fix-user-defaults-parse
Closed

parity: fix UserDefaults json parser#9130
andresilva wants to merge 6 commits into
masterfrom
andre/fix-user-defaults-parse

Conversation

@andresilva

@andresilva andresilva commented Jul 16, 2018

Copy link
Copy Markdown
Contributor

Fixes #8550.

@andresilva andresilva added F2-bug 🐞 The client fails to follow expected behavior. A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 16, 2018
@andresilva andresilva mentioned this pull request Jul 16, 2018
Closed
@ordian

ordian commented Jul 16, 2018

Copy link
Copy Markdown
Member

Dumb question: why can't we use serde derive here instead?

@andresilva

andresilva commented Jul 16, 2018

Copy link
Copy Markdown
Contributor Author

@ordian Seems like this code is a bit old so that's probably the reason. Algorithm and Mode currently do not implement Serialize/Deserialize and it doesn't feel right to add those implementations in util/journaldb/lib.rs or ethcore/src/client/config.rs since those crates don't really care about JSON. The alternative would be to introduce newtypes that wrap mode and algorithm in UserDefaults. Do you think we should do this?

@ordian

ordian commented Jul 16, 2018

Copy link
Copy Markdown
Member

@andresilva I would prefer having derive, because it is more future-proof and less code to read - easier to reason about. As for Algorithm and Mode, we can e.g. use deserialize_with or remote-derive.

@5chdn

5chdn commented Jul 16, 2018

Copy link
Copy Markdown
Contributor

F-Label Society 🤣

@5chdn 5chdn removed the F2-bug 🐞 The client fails to follow expected behavior. label Jul 16, 2018
@5chdn 5chdn added this to the 2.1 milestone Jul 16, 2018
This was referenced Jul 16, 2018
Closed
Merged
@andresilva

andresilva commented Jul 16, 2018

Copy link
Copy Markdown
Contributor Author

@5chdn Indeed 😆 I'm having a hard time learning not to use F-labels on PRs.

@andresilva

andresilva commented Jul 16, 2018

Copy link
Copy Markdown
Contributor Author

@ordian I've updated the PR to use serde_derive. The format of the mode field changed but only this value is lost when upgrading (defaults to active), so I don't think it's much of an issue.

This was referenced Jul 17, 2018
Merged
Merged
ordian
ordian reviewed Jul 17, 2018
View reviewed changes
Comment thread parity/user_defaults.rs Outdated
// would fail. this way we keep the existing values for the other fields and
// only provide a default for the `mode` field in case it fails to
// deserialize.
Ok(Mode::deserialize(deserializer).unwrap_or(Mode::Active))

@ordian ordian Jul 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we can preserve backwards compatibility like this?

@andresilva andresilva Jul 17, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice! But apparently flatten can only be used with structs and maps:

2018-07-17 13:11:47 Error loading user defaults file: ErrorImpl { code: Message("can only flatten structs and maps"), line: 1, column: 107 }

Always prints that when starting although it seems to work. Also the serialization of Duration is different so also not backwards compatible.

(I didn't know you could use crates in rust playground that's super useful!)

@ordian ordian Jul 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, looks like we're using serde 1.0.37 and support for flattening internally tagged enums was only implemented in 1.0.46. The difference in Duration serialization is also probably due to different versions used in playground in parity codebase. UPD: Updated playground link (fixed Duration serialization).

(rust playground was merged with Integer32's playground implementation one year ago)

@ordian ordian added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 17, 2018
@5chdn 5chdn modified the milestones: 2.1, 2.2 Jul 17, 2018
@andresilva andresilva added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Jul 19, 2018
@andresilva

andresilva commented Jul 19, 2018

Copy link
Copy Markdown
Contributor Author

Updated serde and added the Seconds(Duration) wrapper. It should be backwards compatible with the previous format now.

ordian
ordian approved these changes Jul 19, 2018
View reviewed changes

@ordian ordian left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM modulo spaces left

Comment thread parity/user_defaults.rs Outdated
pub struct Seconds(Duration);

impl Seconds {
pub fn value(&self) -> u64 {

@ordian ordian Jul 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are some spaces left from copy-paste, please M-x tabify

@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 19, 2018
@5chdn 5chdn closed this Jul 22, 2018
@5chdn 5chdn reopened this Jul 22, 2018
@5chdn 5chdn closed this Jul 22, 2018
@5chdn 5chdn reopened this Jul 22, 2018
@andresilva andresilva closed this Jul 22, 2018
@andresilva andresilva mentioned this pull request Jul 22, 2018
Merged
@andresilva

andresilva commented Jul 22, 2018

Copy link
Copy Markdown
Contributor Author

@5chdn I've closed this PR and created a new one. It's the easiest way to overcome the CI issue.

@debris debris deleted the andre/fix-user-defaults-parse branch July 23, 2018 11:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@ordian ordian ordian approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.

Projects

None yet

Milestone

2.1 Whatever

Development

Successfully merging this pull request may close these issues.

3 participants

@andresilva @ordian @5chdn