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.

Sign up for GitHub

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

Jump to bottom

Prefer receipt status to code availability on contract deployment #3298

Merged
merged 4 commits into from
Jan 9, 2020
Merged

Prefer receipt status to code availability on contract deployment #3298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
17f72d7
Add deployment test cases for "contract could not be stored"
cgewecke Jan 7, 2020
aec5c08
Prefer receipt status to code size when evaluating deployment success
cgewecke Jan 7, 2020
56a474e
Merge branch '1.x' into fix/get-code-failure
nivida Jan 8, 2020
509d73a
CHANGELOG.md updated
nivida Jan 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,4 @@ Released with 1.0.0-beta.37 code base.
- ``clearSubscriptions`` does no longer throw an error if no running subscriptions do exist (#3246)
- callback type definition for ``Accounts.signTransaction`` fixed (#3280)
- fix: export bloom functions on the index.js
- Prefer receipt status to code availability on contract deployment (#3298)
68 changes: 38 additions & 30 deletions packages/web3-core-method/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,8 @@ Method.prototype._confirmTransaction = function (defer, result, payload) {
isContractDeployment = _.isObject(payload.params[0]) &&
payload.params[0].data &&
payload.params[0].from &&
!payload.params[0].to;
!payload.params[0].to,
hasBytecode = isContractDeployment && payload.params[0].data.length > 2;

// add custom send Methods
var _ethereumCalls = [
Expand Down Expand Up @@ -330,7 +331,7 @@ Method.prototype._confirmTransaction = function (defer, result, payload) {
return receipt;
})
// CHECK for CONTRACT DEPLOYMENT
.then(function (receipt) {
.then(async function (receipt) {

if (isContractDeployment && !promiseResolved) {

Expand All @@ -351,43 +352,50 @@ Method.prototype._confirmTransaction = function (defer, result, payload) {
return;
}

_ethereumCall.getCode(receipt.contractAddress, function (e, code) {

if (!code) {
return;
}

var code;
try {
code = await _ethereumCall.getCode(receipt.contractAddress);
} catch(err){
// ignore;
}

if (code.length > 2) {
defer.eventEmitter.emit('receipt', receipt);
if (!code) {
return;
}

// if contract, return instance instead of receipt
if (method.extraFormatters && method.extraFormatters.contractDeployFormatter) {
defer.resolve(method.extraFormatters.contractDeployFormatter(receipt));
} else {
defer.resolve(receipt);
}
// If deployment is status.true and there was a real
// bytecode string, assume it was successful.
var deploymentSuccess = receipt.status === true && hasBytecode;

// need to remove listeners, as they aren't removed automatically when succesfull
if (canUnsubscribe) {
defer.eventEmitter.removeAllListeners();
}
if (deploymentSuccess || code.length > 2) {
defer.eventEmitter.emit('receipt', receipt);

// if contract, return instance instead of receipt
if (method.extraFormatters && method.extraFormatters.contractDeployFormatter) {
defer.resolve(method.extraFormatters.contractDeployFormatter(receipt));
} else {
utils._fireError(
errors.ContractCodeNotStoredError(receipt),
defer.eventEmitter,
defer.reject,
null,
receipt
);
defer.resolve(receipt);
}

// need to remove listeners, as they aren't removed automatically when succesfull
if (canUnsubscribe) {
sub.unsubscribe();
defer.eventEmitter.removeAllListeners();
}
promiseResolved = true;
});

} else {
utils._fireError(
errors.ContractCodeNotStoredError(receipt),
defer.eventEmitter,
defer.reject,
null,
receipt
);
}

if (canUnsubscribe) {
sub.unsubscribe();
}
promiseResolved = true;
}

return receipt;
Expand Down
71 changes: 62 additions & 9 deletions test/e2e.contract.deploy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ describe('contract.deploy [ @E2E ]', function() {
var accounts;
var basic;
var reverts;
var noBytecode;
var options;

// Error message variants
var ganacheRevert = "revert";
var gethRevert = "code couldn't be stored";
var revertMessage = "revert";
var couldNotBeStoredMessage = "code couldn't be stored";
var creationWithoutDataMessage = "contract creation without any data provided";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This message emitted by geth, code ref here

https://github.com/ethereum/go-ethereum/blob/7a509b47321a28e36f437170a3cc5c283ee7909d/internal/ethapi/api.go#L1404-L1406


var basicOptions = {
data: Basic.bytecode,
Expand All @@ -27,13 +29,20 @@ describe('contract.deploy [ @E2E ]', function() {
gas: 4000000
}

var noBytecodeOptions = {
data: '0x',
gasPrice: '1',
gas: 4000000
}

describe('http', function() {
before(async function(){
web3 = new Web3('http://localhost:8545');
accounts = await web3.eth.getAccounts();

basic = new web3.eth.Contract(Basic.abi, basicOptions);
reverts = new web3.eth.Contract(Reverts.abi, revertsOptions);
noBytecode = new web3.eth.Contract(Basic.abi, noBytecodeOptions);
})

it('returns an instance', async function(){
Expand All @@ -44,7 +53,9 @@ describe('contract.deploy [ @E2E ]', function() {
assert(web3.utils.isAddress(instance.options.address));
});

it('errors on OOG', async function(){
// Clients reject this kind of OOG is early because
// the gas is obviously way too low.
it('errors on "intrinic gas too low" OOG', async function(){
try {
await basic
.deploy()
Expand All @@ -53,9 +64,49 @@ describe('contract.deploy [ @E2E ]', function() {
assert.fail();
} catch(err){
assert(err.message.includes('gas'))
assert(err.receipt === undefined);
}
});

// Clients reject this kind of OOG when the EVM runs out of gas
// while running the code. A contractAddress is set on the
// receipt, but the status will be false.
it('errors on OOG reached while running EVM', async function(){
const estimate = await basic
.deploy()
.estimateGas()

const gas = estimate - 1000;

try {
await basic
.deploy()
.send({from: accounts[0], gas: gas});

assert.fail();
} catch(err){
assert(err.message.includes(couldNotBeStoredMessage));
assert(err.receipt.status === false);
}
});

// Geth immediately rejects a zero length bytecode without mining,
// Ganache accepts and mines it, returning a receipt with status === true
it('errors deploying a zero length bytecode', async function(){
try {
await noBytecode
.deploy()
.send({from: accounts[0]});

assert.fail();
} catch(err){
assert(
err.message.includes(creationWithoutDataMessage) ||
err.message.includes(couldNotBeStoredMessage)
);
}
})

it('errors on revert', async function(){
try {
await reverts
Expand All @@ -65,9 +116,11 @@ describe('contract.deploy [ @E2E ]', function() {
assert.fail();
} catch(err){
assert(
err.message.includes(gethRevert) ||
err.message.includes(ganacheRevert)
err.message.includes(couldNotBeStoredMessage) ||
err.message.includes(revertMessage)
);

assert(err.receipt.status === false);
}
});
});
Expand Down Expand Up @@ -115,8 +168,8 @@ describe('contract.deploy [ @E2E ]', function() {
assert.fail();
} catch(err){
assert(
err.message.includes(gethRevert) ||
err.message.includes(ganacheRevert)
err.message.includes(couldNotBeStoredMessage) ||
err.message.includes(revertMessage)
);
}
});
Expand Down Expand Up @@ -177,8 +230,8 @@ describe('contract.deploy [ @E2E ]', function() {
.send({from: accounts[0]})
.on('error', err => {
assert(
err.message.includes(gethRevert) ||
err.message.includes(ganacheRevert)
err.message.includes(couldNotBeStoredMessage) ||
err.message.includes(revertMessage)
);
done();
})
Expand Down