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

Fixing pricing-policy ID 0 for some farms #164

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

sameh-farouk
Copy link
Member

@sameh-farouk sameh-farouk commented Apr 22, 2024

There is an issue of data inconsistency caused by an old implementation of the update farm extrinsic. This allowed users to mess up with their farms' pricing policy ID. Later, a storage migration reset all the farms' pricing policy IDs to the default one.
To solve this issue, I propose that the value of pricing policy ID on all farmUpdated events shouldn't persist since it was never meant to be altered in the first place, and with the current chain implantation, we are not allowing such change.

For full context see here
#96 (comment)

@sameh-farouk sameh-farouk requested a review from renauter as a code owner April 22, 2024 10:43
Copy link
Contributor

@renauter renauter left a comment

Choose a reason for hiding this comment

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

Are we sure pricing_policy_id on farm was always initialized to 1 ?
If not we could just set savedFarm.pricingPolicyID = 1 to force instead of commenting savedFarm.pricingPolicyID = farmUpdatedEventParsed.pricingPolicyId

@sameh-farouk
Copy link
Member Author

Are we sure pricing_policy_id on farm was always initialized to 1 ?

yes, I checked and farms was created with a hardcoded farming policy ID 1 since the initial implementation of this extrinsic.

If not we could just set savedFarm.pricingPolicyID = 1 to force instead of commenting savedFarm.pricingPolicyID = farmUpdatedEventParsed.pricingPolicyId

IMO, both are workable with the current implementation of tfgridmodule, but it is a matter of taste, I don't prefer to use a magic value here, and it is not the responsibility of the processor to decide about the value (and you could argue that my approach also decide about the value by ignoring it but this taste better for me :) ).
Also, the approach I choose here by keeping the original pricing policy attached to the farm when it was created, will still be functioning properly even if we decide to change the hardcoded value on farm creation to something else.

@sameh-farouk sameh-farouk merged commit c4a50cc into master Apr 22, 2024
1 check passed
@sameh-farouk sameh-farouk deleted the master-fix-pricing-policy-id-0 branch April 22, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Farm pricing policy not in sync
2 participants