Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix wrong transaction input for contract deployments #4052

Merged
merged 5 commits into from
Jan 6, 2017
Merged

Fix wrong transaction input for contract deployments #4052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dbb6433
Fix mutable options in Contract API
ngotchac Jan 5, 2017
43d9f22
Merge branch 'master' into ng-fix-deploy
ngotchac Jan 5, 2017
ab9b5d2
Add Swarm hash and meta data from Solidity
ngotchac Jan 5, 2017
61dc4f2
Updates tests for contract deployment
ngotchac Jan 5, 2017
49d4403
Add test for deploy without constructor Params
ngotchac Jan 5, 2017
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
15 changes: 10 additions & 5 deletions js/src/api/contract/contract.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,14 +218,19 @@ export default class Contract {
}

_encodeOptions (func, options, values) {
options.data = this.getCallData(func, options, values);
return options;
const data = this.getCallData(func, options, values);
Copy link
Contributor

@jacogr jacogr Jan 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Since we fixed a bug, would love a testcase that reproduces it and verifies it fixed so it doesn't happen again. (In the case of fixing bugs, should get into that habit overall)


return {
...options,
data
};
}

_addOptionsTo (options = {}) {
return Object.assign({
to: this._address
}, options);
return {
to: this._address,
...options
};
}

_bindFunction = (func) => {
Expand Down
72 changes: 65 additions & 7 deletions js/src/api/contract/contract.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const eth = new Api(transport);

describe('api/contract/Contract', () => {
const ADDR = '0x0123456789';

const ABI = [
{
type: 'function', name: 'test',
Expand All @@ -41,12 +42,42 @@ describe('api/contract/Contract', () => {
type: 'function', name: 'test2',
outputs: [{ type: 'uint' }, { type: 'uint' }]
},
{ type: 'constructor' },
{
type: 'constructor',
inputs: [{ name: 'boolin', type: 'bool' }, { name: 'stringin', type: 'string' }]
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. Small grumble...

Would have preferred to keep a non-parameters constructor in addition to the new with-parameters constructor (i.e. separate tests). The no-parameters tests obviously worked (maybe named incorrectly), it was the with-parameters case that was missing.

Basically if things ever go haywire with some extra stuff added to constructors where we expect none, we won't catch it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

},
{ type: 'event', name: 'baz' },
{ type: 'event', name: 'foo' }
];
const VALUES = [true, 'jacogr'];
const ENCODED = '0x023562050000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000066a61636f67720000000000000000000000000000000000000000000000000000';

const ABI_NO_PARAMS = [
{
type: 'function', name: 'test',
inputs: [{ name: 'boolin', type: 'bool' }, { name: 'stringin', type: 'string' }],
outputs: [{ type: 'uint' }]
},
{
type: 'function', name: 'test2',
outputs: [{ type: 'uint' }, { type: 'uint' }]
},
{
type: 'constructor'
},
{ type: 'event', name: 'baz' },
{ type: 'event', name: 'foo' }
];

const VALUES = [ true, 'jacogr' ];
const CALLDATA = `
0000000000000000000000000000000000000000000000000000000000000001
0000000000000000000000000000000000000000000000000000000000000040
0000000000000000000000000000000000000000000000000000000000000006
6a61636f67720000000000000000000000000000000000000000000000000000
`.replace(/\s/g, '');
const SIGNATURE = '02356205';

const ENCODED = `0x${SIGNATURE}${CALLDATA}`;

const RETURN1 = '0000000000000000000000000000000000000000000000000000000000123456';
const RETURN2 = '0000000000000000000000000000000000000000000000000000000000456789';
let scope;
Expand Down Expand Up @@ -230,6 +261,33 @@ describe('api/contract/Contract', () => {
});
});

describe('deploy without parameters', () => {
const contract = new Contract(eth, ABI_NO_PARAMS);
const CODE = '0x123';
const ADDRESS = '0xD337e80eEdBdf86eDBba021797d7e4e00Bb78351';
const RECEIPT_DONE = { contractAddress: ADDRESS.toLowerCase(), gasUsed: 50, blockNumber: 2500 };

let scope;

describe('success', () => {
before(() => {
scope = mockHttp([
{ method: 'eth_estimateGas', reply: { result: 1000 } },
{ method: 'parity_postTransaction', reply: { result: '0x678' } },
{ method: 'parity_checkRequest', reply: { result: '0x890' } },
{ method: 'eth_getTransactionReceipt', reply: { result: RECEIPT_DONE } },
{ method: 'eth_getCode', reply: { result: CODE } }
]);

return contract.deploy({ data: CODE }, []);
});

it('passes the options through to postTransaction (incl. gas calculation)', () => {
expect(scope.body.parity_postTransaction.params[0].data).to.equal(CODE);
});
});
});

describe('deploy', () => {
const contract = new Contract(eth, ABI);
const ADDRESS = '0xD337e80eEdBdf86eDBba021797d7e4e00Bb78351';
Expand All @@ -252,7 +310,7 @@ describe('api/contract/Contract', () => {
{ method: 'eth_getCode', reply: { result: '0x456' } }
]);

return contract.deploy({ data: '0x123' }, []);
return contract.deploy({ data: '0x123' }, VALUES);
});

it('calls estimateGas, postTransaction, checkRequest, getTransactionReceipt & getCode in order', () => {
Expand All @@ -261,7 +319,7 @@ describe('api/contract/Contract', () => {

it('passes the options through to postTransaction (incl. gas calculation)', () => {
expect(scope.body.parity_postTransaction.params).to.deep.equal([
{ data: '0x123', gas: '0x4b0' }
{ data: `0x123${CALLDATA}`, gas: '0x4b0' }
]);
});

Expand All @@ -280,7 +338,7 @@ describe('api/contract/Contract', () => {
]);

return contract
.deploy({ data: '0x123' }, [])
.deploy({ data: '0x123' }, VALUES)
.catch((error) => {
expect(error.message).to.match(/not deployed, gasUsed/);
});
Expand All @@ -296,7 +354,7 @@ describe('api/contract/Contract', () => {
]);

return contract
.deploy({ data: '0x123' }, [])
.deploy({ data: '0x123' }, VALUES)
.catch((error) => {
expect(error.message).to.match(/not deployed, getCode/);
});
Expand Down
46 changes: 44 additions & 2 deletions js/src/views/WriteContract/writeContract.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,23 +458,65 @@ class WriteContract extends Component {
const { bytecode } = contract;
const abi = contract.interface;

const metadata = contract.metadata
? (
<Input
allowCopy
label='Metadata'
readOnly
value={ contract.metadata }
/>
)
: null;

return (
<div>
<Input
allowCopy
label='ABI Interface'
readOnly
value={ abi }
label='ABI Interface'
/>

<Input
allowCopy
label='Bytecode'
readOnly
value={ `0x${bytecode}` }
label='Bytecode'
/>

{ metadata }
{ this.renderSwarmHash(contract) }
</div>
);
}

renderSwarmHash (contract) {
if (!contract || !contract.metadata) {
return null;
}

const { bytecode } = contract;

// @see https://solidity.readthedocs.io/en/develop/miscellaneous.html#encoding-of-the-metadata-hash-in-the-bytecode
const hashRegex = /a165627a7a72305820([a-f0-9]{64})0029$/;

if (!hashRegex.test(bytecode)) {
return null;
}

const hash = hashRegex.exec(bytecode)[1];

return (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

<Input
allowCopy
label='Swarm Metadata Hash'
readOnly
value={ `${hash}` }
/>
);
}

renderErrors () {
const { annotations } = this.store;

Expand Down