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 reward and treasury settings to settings REST #1291

Merged
merged 2 commits into from Dec 7, 2019
Merged

Conversation

CodeSandwich
Copy link
Contributor

Fixes #1248

@CodeSandwich CodeSandwich added subsys-rest issues or PRs for the REST interface A-doc Area: Documentation labels Dec 5, 2019
@NicolasDP NicolasDP added the DO NOT MERGE not to be merged until something else is done label Dec 5, 2019
doc/jcli/rest.md Outdated Show resolved Hide resolved
jormungandr-lib/src/interfaces/settings.rs Show resolved Hide resolved
jormungandr-lib/src/interfaces/settings.rs Show resolved Hide resolved
Comment on lines +86 to +88
#[derive(Deserialize, Serialize)]
#[serde(transparent, remote = "value::Value")]
pub struct ValueDef(u64);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not going to be always true that a Value is a u64. Use the Value type above and pay the cost of writing .into().

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 is guaranteed by Serde's remote attribute, see https://serde.rs/remote-derive.html

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't see the point in duplicating that Value. I see how it makes it easy. for you but I'd rather like to see all of this going toward a harmonised interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All fields in SettingsDto are "normal" types from chain-impl-mockchain, but with custom serialization (mostly with and remote types). This make its usage easier, especially in tests. Adding remote type for Value lets us keep this property of SettingsDto.

I also think that its a cleaner solution than conversion from and to the jormungandr-lib::Value and it's generally a good idea to start using it in all Serde DTOs.

jormungandr/src/rest/v0/handlers.rs Outdated Show resolved Hide resolved
doc/openapi.yaml Outdated Show resolved Hide resolved
@NicolasDP NicolasDP added enhancement New feature or request A-jormungandr Area: Issues affecting jörmungandr and removed DO NOT MERGE not to be merged until something else is done labels Dec 6, 2019
@NicolasDP NicolasDP merged commit 41380a3 into master Dec 7, 2019
@NicolasDP NicolasDP deleted the rest_more branch December 7, 2019 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-doc Area: Documentation A-jormungandr Area: Issues affecting jörmungandr enhancement New feature or request subsys-rest issues or PRs for the REST interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REST: update /api/v0/settings
2 participants