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

fix: save graced contracts total amount due #631

Merged
merged 5 commits into from
Mar 9, 2023
Merged

Conversation

DylanVerstraete
Copy link
Contributor

No description provided.

Copy link
Contributor

@brandonpille brandonpille left a comment

Choose a reason for hiding this comment

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

Nice catch.

Copy link
Collaborator

@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.

Great!
Meanwhile I have some comments

substrate-node/pallets/pallet-smart-contract/src/lib.rs Outdated Show resolved Hide resolved
substrate-node/pallets/pallet-smart-contract/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 1190 to 1193
if matches!(contract.state, types::ContractState::Deleted(_)) {
Self::remove_contract(contract.contract_id);
return Ok(().into());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the bill_contract() context, the only way to have a contract in Deleted state is just after the handle_grace()
So i guess this block can be moved above
With this you can get rid of the if at line 1157

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the contract is also moved to state Deleted if the user cancels it:

Self::_update_contract_state(&mut contract, &types::ContractState::Deleted(cause))?;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, I don't understand why there is a Deleted state for contract because I don't see something specific done with this info
Just removing the contract would not be fine?
or I miss something

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, the contract is also moved to state Deleted if the user cancels it:

Self::_update_contract_state(&mut contract, &types::ContractState::Deleted(cause))?;

Ok, I see
So even when Deleted it is billed and need to reach the end of bill_contract()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, I don't understand why there is a Deleted state for contract because I don't see something specific done with this info Just removing the contract would not be fine? or I miss something

It's a leftover state from where we did not remove contracts from chain but marked them as deleted for ever. We can in a next iteration remove this maybe?

@DylanVerstraete DylanVerstraete merged commit f0ee553 into development Mar 9, 2023
@DylanVerstraete DylanVerstraete deleted the fix_grace branch March 9, 2023 16:17
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.

4 participants