search

LiskArchive / lisk-sdk

🔩 Lisk software development kit
https://lisk.com
Apache License 2.0
2.72k stars 456 forks source link

Refactor the `update` function of the Application State to make it generic #3875

Closed 2snEM6 closed 5 years ago

2snEM6 commented 5 years ago

Expected behavior

The implementation should be written in a more generic way, allowing the user to update whichever property is available and updateable in the state.

For that, we could define an Application State schema that defines which properties are read-only, their formats, etc.

update() would validate this schema and override these properties if it is allowed with the values passed in the arguments.

File: framework/src/controller/application_state.js

Actual behavior

The update function of the Application State is currently forcing to update certain properties every time is called. There are certain situations where we want to update one property and not the one that is being enforced.

The implementation is too specific and tied to those properties. See: https://github.com/LiskHQ/lisk-sdk/pull/3900#discussion_r299879856

Which version(s) does this affect? (Environment, OS, etc...)

feature/development

pablitovicente commented 5 years ago

I would also consider using getter/setters for this instead of an schema.

2snEM6 commented 5 years ago

Thanks @pablitovicente. Isn't it easier to maintain a fairly simple and short schema that takes care of all of this at once by using JSON Schema definition that having to use getters and setters individually?

pablitovicente commented 5 years ago

But with getter/setters we have kind of proxies if in the future we need to perform additional operations upon an application state change but if we have an schema we have to validate and then use branching for doing the same.

jondubois commented 5 years ago

I think this is a duplicate of https://github.com/LiskHQ/lisk-sdk/issues/3222 - The P2P team has picked it up for the current sprint. Can we close this issue?

About the schema, currently P2P relies on a flexible schema with custom properties so we should also make it flexible in the application state. Also, the application state will only be updated internally by trusted modules running on the current node so we don't need to do validation.