Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Update forging enable to be more strict #6674

Closed
shuse2 opened this issue Aug 23, 2021 · 9 comments · Fixed by #6766
Closed

Update forging enable to be more strict #6674

shuse2 opened this issue Aug 23, 2021 · 9 comments · Fixed by #6766

Comments

@shuse2
Copy link
Collaborator

shuse2 commented Aug 23, 2021

Description

Forging enable should be strict for case

height: 0
maxHeightPrevoted: 0
maxHeightPreviouslyForged: 0
  • case 1: Input is 0, 0, 0 and there is no previous forging data
    expected result: ok
  • case 2: Input is 0, 0, 0 and there is non zero previous forging data
    expected result: fail
  • case 3: Input is 0, 0, 0 and there is non zero previous forging data with overwrite flag
    expected result: ok
  • case 4: Input is x, y, z and there is non zero previous forging data
    expected result: fail
  • case 5: Input is x, y, z and there is non zero previous forging data with overwrite flag
    expected result: ok

Acceptance Criteria

  • All above cases are tested
@ManuGowda
Copy link
Contributor

maybe we can also fix the order of fields to match forging:status command?

@shuse2
Copy link
Collaborator Author

shuse2 commented Aug 23, 2021

which order are u referring to?

@ManuGowda
Copy link
Contributor

[{"address":"xyz","forging":true,"height":0,"maxHeightPrevoted":0,"maxHeightPreviouslyForged":0}]
lisk-core forging:enable ADDRESS HEIGHT MAXHEIGHTPREVIOUSLYFORGED MAXHEIGHTPREVOTED

sometimes users run forgins:status to enable that time, copy paste could lead to errors, so if the order of MAXHEIGHTPREVIOUSLYFORGED MAXHEIGHTPREVOTED is same then its easy to copy and paste?

@shuse2
Copy link
Collaborator Author

shuse2 commented Aug 23, 2021

JSON object key is unordered, therefore, we cannot guarantee that is will be in that order if we use JSON output.
If we change the output format, it will be breaking change

@ManuGowda
Copy link
Contributor

no we dont have to change the forging:status output, we can just update the forging:enable input order to match it?

@dakk
Copy link
Contributor

dakk commented Aug 23, 2021

no we dont have to change the forging:status output, we can just update the forging:enable input order to match it?

bad idea in my opinion, this will break existing tools

EDIT:
a better idea could be to allow forging:enable to [optionally] gather right values from lisk-core as we do manually, so the procedure to enable forging is less prone to errors

@shuse2
Copy link
Collaborator Author

shuse2 commented Aug 23, 2021

@dakk what do you mean by right value? potentially lisk-core forging:status can return a valid value, if the data is migrated properly, but it's not possible to know if the data is migrated properly or not 😓

@dakk
Copy link
Contributor

dakk commented Aug 23, 2021

@dakk what do you mean by right value? potentially lisk-core forging:status can return a valid value, if the data is migrated properly, but it's not possible to know if the data is migrated properly or not sweat

I mean, lisk-core forging:enable should be able to get automatically values from forging:status; not as default, but by passing an optional parameter. It's up to the delegate to check if he/she want to use that option (like the overwrite), or if he/she want to provide the values.

Something like this:

lisk-core forging:enable --use-status-values

@JesusTheHun
Copy link
Contributor

@shuse2 a responsible delegate will run lisk-core forging:status first to make sure the migration was successful, then use lisk-core forging:enable --use-status-values

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants