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

Contracts: Remove ED from base deposit #3536

Merged
merged 17 commits into from
Apr 10, 2024
Merged

Conversation

pgherveou
Copy link
Contributor

@pgherveou pgherveou commented Mar 1, 2024

  • Update internal logic so that the storage_base_deposit does not include ED
  • add v16 migration to update ContractInfo struct with this change

Before:
Screenshot 2024-03-21 at 11 23 29

After:
Screenshot 2024-03-21 at 11 23 42

@pgherveou pgherveou requested a review from athei as a code owner March 1, 2024 08:28
@pgherveou pgherveou added the R0-silent Changes should not be mentioned in any release notes label Mar 1, 2024
@pgherveou
Copy link
Contributor Author

@athei, I just reviewed this code, I wonder if we should use max here instead of adding ED
if not then the nit updates in this PR remove the unrelevant comments

// Instantiate needs to transfer at least the minimum balance in order to pull the
// contract's own account into existence, as the deposit itself does not contribute to the
// `ed`.
let deposit = info_deposit.saturating_add(upload_deposit).saturating_add(ed);

@athei
Copy link
Member

athei commented Mar 1, 2024

I think this is a left over when we had a separate account for deposits. Then the deposit was free and could keep the deposit account alive. Hence we needed to make sure that it is high enough. Now we have the deposits on the main account which has always gets the ed on instantiate:

// We need to make sure that the contract's account exists.
T::Currency::transfer(origin, contract, ed, Preservation::Preserve)?;

AFAIK we neither need max nor add here.

@pgherveou
Copy link
Contributor Author

I think this is a left over when we had a separate account for deposits. Then the deposit was free and could keep the deposit account alive. Hence we needed to make sure that it is high enough. Now we have the deposits on the main account which has always gets the ed on instantiate:

// We need to make sure that the contract's account exists.
T::Currency::transfer(origin, contract, ed, Preservation::Preserve)?;

AFAIK we neither need max nor add here.

Yep that make sense, will amend the PR

@pgherveou
Copy link
Contributor Author

well actually, never mind this is correct, we just take into account the ED in the base and don't charge for it twice.

/~https://github.com/paritytech/polkadot-sdk/blob/6f81a4a09270a5930b8786e2dc765347078b4d4a/substrate/frame/contracts/src/storage/meter.rs?plain=1#L461j

@athei
Copy link
Member

athei commented Mar 2, 2024

Yes it seems correct but it seems to be redundant to add it just to remove it again. The deposit also can be smaller than the ed with no problem. I think it is leftover code and we can just clean it up.

@pgherveou pgherveou changed the title Contracts Simplify test Contracts: Remove ED from base deposit Mar 20, 2024
@pgherveou
Copy link
Contributor Author

bot bench substrate-pallet --pallet=pallet_contracts

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Good catch. Yes the deposit has to add up to the reserved amount.

One note: Even with the migration the reserved amount might have a mismatch with the recorded deposit. Reason is that we probably refunded a tiny bit too much when refunding. Because we assumed that we had more deposit than we actually had. But I am not sure about that.

@pgherveou pgherveou requested a review from xermicus April 10, 2024 12:39
substrate/frame/contracts/src/migration/v16.rs Outdated Show resolved Hide resolved
@pgherveou pgherveou force-pushed the pg/contracts-simplify-test branch from 5ea5888 to 0014dff Compare April 10, 2024 16:21
@pgherveou pgherveou changed the base branch from master to pg/only-validate-when-uploading April 10, 2024 16:22
@pgherveou
Copy link
Contributor Author

bot bench substrate-pallet --pallet=pallet_contracts

@command-bot
Copy link

command-bot bot commented Apr 10, 2024

@pgherveou https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5869709 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_contracts. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 6-be124f7e-37bd-48e1-89cb-362295c36ef6 to cancel this command or bot cancel to cancel all commands in this pull request.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5869579

@pgherveou
Copy link
Contributor Author

bot bench substrate-pallet --pallet=pallet_contracts

@command-bot
Copy link

command-bot bot commented Apr 10, 2024

@pgherveou https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5872017 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_contracts. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 7-e250f04b-0a70-48c7-9866-900d0be3affb to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Apr 10, 2024

@pgherveou Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5869709 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5869709/artifacts/download.

…=dev --target_dir=substrate --pallet=pallet_contracts
@command-bot
Copy link

command-bot bot commented Apr 10, 2024

@pgherveou Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5872017 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5872017/artifacts/download.

Base automatically changed from pg/only-validate-when-uploading to master April 10, 2024 19:19
@pgherveou pgherveou enabled auto-merge April 10, 2024 19:28
@pgherveou pgherveou added this pull request to the merge queue Apr 10, 2024
Merged via the queue into master with commit 643aa2b Apr 10, 2024
129 of 135 checks passed
@pgherveou pgherveou deleted the pg/contracts-simplify-test branch April 10, 2024 21:28
@pgherveou
Copy link
Contributor Author

bot help

@command-bot
Copy link

command-bot bot commented Apr 12, 2024

Here's a link to docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants