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

Add MinimumDeploymentFee #2140

Merged
merged 6 commits into from
Dec 15, 2020
Merged

Add MinimumDeploymentFee #2140

merged 6 commits into from
Dec 15, 2020

Conversation

erikzhang
Copy link
Member

@erikzhang erikzhang commented Dec 11, 2020

Close #2001

@erikzhang
Copy link
Member Author

@roman-khimov Please check it.

@@ -44,6 +50,20 @@ internal override void OnPersist(ApplicationEngine engine)
}
}

[ContractMethod(0_01000000, CallFlags.ReadStates)]
private long GetMinimumDeploymentFee(StoreView snapshot)
Copy link
Contributor

Choose a reason for hiding this comment

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

And maybe it's better to have this value in Policy contract.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure. This is a parameter related to contract deployment, and maybe it should be placed here.

Copy link
Member

Choose a reason for hiding this comment

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

I think like @roman-khimov , it's related to the environment. But also it's ok too

@@ -10,7 +10,7 @@ namespace Neo.UnitTests.Extensions
{
public static class NativeContractExtensions
{
public static ContractState DeployContract(this StoreView snapshot, UInt160 sender, byte[] nefFile, byte[] manifest, long gas = ApplicationEngine.TestModeGas)
public static ContractState DeployContract(this StoreView snapshot, UInt160 sender, byte[] nefFile, byte[] manifest, long gas = 200_00000000)
Copy link
Member

Choose a reason for hiding this comment

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

it's 100?

Suggested change
public static ContractState DeployContract(this StoreView snapshot, UInt160 sender, byte[] nefFile, byte[] manifest, long gas = 200_00000000)
public static ContractState DeployContract(this StoreView snapshot, UInt160 sender, byte[] nefFile, byte[] manifest, long gas = 100_00000000)

@@ -44,6 +50,20 @@ internal override void OnPersist(ApplicationEngine engine)
}
}

[ContractMethod(0_01000000, CallFlags.ReadStates)]
private long GetMinimumDeploymentFee(StoreView snapshot)
Copy link
Member

Choose a reason for hiding this comment

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

I think like @roman-khimov , it's related to the environment. But also it's ok too

@erikzhang
Copy link
Member Author

Merge?

@shargon
Copy link
Member

shargon commented Dec 15, 2020

@Tommo-L proposed to reduce the min fee to 1, what do you think?

#2001 (comment)

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

we can adjust the value later

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

It should be on the Police too.

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

But if it is just 1 GAS then I think it would be ok here.

@erikzhang erikzhang added this to the NEO 3.0 milestone Dec 15, 2020
@erikzhang erikzhang merged commit 06d4345 into master Dec 15, 2020
@erikzhang erikzhang deleted the min-deployment-fee branch December 15, 2020 13:46
@Tommo-L
Copy link
Contributor

Tommo-L commented Dec 15, 2020

I see we can adjust it later, but it needs the multi-signatures of committee. And 100GAS is still higher than Ethereum, which is about 0.04 ~ 0.2 ETH(~ 16GAS - 80GAS) for a simple ERC20 contract.

@Tommo-L
Copy link
Contributor

Tommo-L commented Dec 15, 2020

I think this way will be better.

DeployFee= baseFee + Factor * engine.StoragePrice * (nefFile.Length + manifest.Length)

Like:
baseFee, 1 ~ 10GAS
Factor, 1 ~ 5

Perhaps we can do a test with the Nep17 template.

@superboyiii
Copy link
Member

We should not adjust MinimumDeploymentFee to such a high level. It's not friendly to developers although I know it could reduce malicious attack. Maybe we could adjust it to 10 GAS since the price for OPcode already makes sense now. @erikzhang

@Tommo-L
Copy link
Contributor

Tommo-L commented Dec 16, 2020

The size of NEP17 template is about 4KB, nef 2KB, manifest 2KB, which needs about 4GAS in the old way.
In ethereum, a simple ERC20 contract is about 10KB, which needs 0.034ETH ~ 20$ ~ 14GAS.

@superboyiii
Copy link
Member

I like the old way. If for malicious attack concern, we could increase that to some extent but should not be so high.

@Tommo-L Tommo-L mentioned this pull request Dec 16, 2020
ShawnYun pushed a commit to ShawnYun/neo that referenced this pull request Jan 8, 2021
cloud8little pushed a commit to cloud8little/neo that referenced this pull request Jan 24, 2021
ixje added a commit to CityOfZion/neo-mamba that referenced this pull request Feb 3, 2021
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.

Contract deployment price coefficient and minimum value
6 participants