Decoding logs with bytes data fields broken in 1.2.8 for solc 0.4.x / external #3544
Comments
|
Hey @bingen thanks for reporting, I will take a closer look this afternoon to see if we have tests for decoding logs with bytes data and if it’s working properly. Meanwhile, could you share what the raw data is for the bytes field in your example above? It seems the ethers abi package is erroring out on “data out-of-bounds”. |
|
Yes, it was: Thanks! |
|
hey @bingen, it does look like we have tests for decodeLog that include bytes examples and they are passing ok: https://github.com/ethereum/web3.js/blob/1.x/test/eth.abi.decodeLog.js When I try to add a test with your example data: {
params: [[{
type: 'bytes',
name: 'data'
}], '0x00000000000000000000000000000000000000000000000000000000000000280000000000000000000000000000000000000000000000000000000000000078000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000040cbd8c2f'],
result: {
'data': '0x00000000000000000000000000000000000000000000000000000000000000280000000000000000000000000000000000000000000000000000000000000078000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000040cbd8c2f',
"__length__": 1
}
}I get: |
|
Hm, but that data is for the whole log, which has 2 uints and 1 bytes fields, so it’s: |
|
@bingen ah ok thanks that makes more sense. Ok I can reproduce the same error now with |
|
It’s supposed to be a function signature: https://github.com/aragon/staking/blob/master/test/lock_and_call.js#L27 |
|
@bingen great thanks, ok looks like there may be some issue in the transport of the event because the function signature for {
params: ['bytes', '0xabe985cb'],
result: '0x00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000004abe985cb00000000000000000000000000000000000000000000000000000000'
}It could be an encoding/decoding issue in the middle, will have to keep digging deeper on this one. We upgraded our abi coder dependency in 1.2.8 from ethers v4 to v5 so we are especially sensitive to any issues. |
|
Oh, yes, sorry, that’s because I changed its name, it used to be |
|
@bingen It's possible this is a compatibility issue with Truffle which remains on Web3 1.2.1 (and Ethers V4).
|
|
Have tried to make a reproduction case for this using code from aragon contract-helpers, Truffle and Web3 1.2.8 and was not able to trigger the error. Could you look at https://github.com/cgewecke/web3-issue-3544 and see if it models the bug correctly? One notable difference from what's reported here is that when I have a solidity event like ...and inspect the receipt from a transaction that fires it, the raw log data looks like this: 0x0000000000000000000000000000000000000000000000000000000000000005000000000000000000000000000000000000000000000000000000000000000500000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000004abe985cb00000000000000000000000000000000000000000000000000000000 It's right padded. Is it possible your data was getting mangled somewhere after you got the receipt? |
|
@bingen Apologies, did you have a chance to look at this again? We'd really like to fix this ASAP if it's happening but at the moment are having difficulty reproducing it from a real example. |
|
Hey, sorry, I didn’t have time to look at this, I’ll try tomorrow. |
|
Hm, this is weird, I tired a fresh clone of the repo using this new branch where I just upgrade to web3 Besides, your example above looks good to me. |
|
Sweet! Thanks @bingen. |
|
Hey, sorry to bother with this again, but there was a problem in the branch I linked above: I have fixed it, so it’s all And I see those errors again. You should see them too if you follow the steps mentioned above: The errors are: I’m going to try to see if I can provide any more info about it. PS: I don’t see the option to reopen the issue, maybe you can do it if you think it’s a web3.js problem. |
|
I see some difference between the logged data in your test and what I get adding the same
Let’s break it down: vs So it seems the problem is I’m missing those trailing zeroes, and hence the buffer overrun. |
|
@bingen Was able to trigger the error in the minimal reproduction at https://github.com/cgewecke/web3-issue-3544 with this commit by
It's not reproducible in solc 0.5.x and the underlying issue was documented here: ethereum/solidity#3493. So...on one hand the Ethers ABICoder upgrade has broken what used to work, on the other hand this discrepancy in the way solc encodes based on function visibility is weird. What is your view of this? |
|
Oops, yes, what a bug. I wonder why it was working with previous versions if the logs were coming different. Do you know how that difference was being handled? |
|
Hi! I'm having the same issue when trying to decode uniswap 2 pair contracts swap logs (solidity 0.5.16) with Data field: Full Error Log: |
|
@geomad Thanks for reporting. Could you provide more specific info about:
Are you using this?
Event signatures in the etherscan contract referenced above: event Mint(address indexed sender, uint amount0, uint amount1);
event Burn(address indexed sender, uint amount0, uint amount1, address indexed to);
event Swap(
address indexed sender,
uint amount0In,
uint amount1In,
uint amount0Out,
uint amount1Out,
address indexed to
);
event Sync(uint112 reserve0, uint112 reserve1);Solidity version is |
|
@cgewecke Exactly. I'm trying to decode the Swap event Log above. |
|
Was unable to reproduce your error while interacting with the deployed Uniswap contracts. For example, decoding the Swap event for mainnet tx: "0x4f19e76573f363956e9e356fdce0b65b6610f6c6663f22b1c21171f05363698c" const abi = [
{
"indexed": true,
"internalType": "address",
"name": "sender",
"type": "address"
},
{
"indexed": false,
"internalType": "uint256",
"name": "amount0In",
"type": "uint256"
},
{
"indexed": false,
"internalType": "uint256",
"name": "amount1In",
"type": "uint256"
},
{
"indexed": false,
"internalType": "uint256",
"name": "amount0Out",
"type": "uint256"
},
{
"indexed": false,
"internalType": "uint256",
"name": "amount1Out",
"type": "uint256"
},
{
"indexed": true,
"internalType": "address",
"name": "to",
"type": "address"
}
];
const tx = "0x4f19e76573f363956e9e356fdce0b65b6610f6c6663f22b1c21171f05363698c";
const web3 = new Web3('https://mainnet.infura.io/v3/<INFURA_ID>');
const receipt = await web3.eth.getTransactionReceipt(tx);
const result = web3.eth.abi.decodeLog(
abi,
receipt.logs[4].data,
receipt.logs[4].topics.slice(1)
);
> Result {
'0': '0xf164fC0Ec4E93095b804a4795bBe1e041497b92a',
'1': '0',
'2': '1040000000000000000',
'3': '801266216497835763803',
'4': '0',
'5': '0xaaa2e80AB7D7b3C216af30Fc8165E7823e74cc62',
__length__: 6,
sender: '0xf164fC0Ec4E93095b804a4795bBe1e041497b92a',
amount0In: '0',
amount1In: '1040000000000000000',
amount0Out: '801266216497835763803',
amount1Out: '0',
to: '0xaaa2e80AB7D7b3C216af30Fc8165E7823e74cc62'
}Does this model your case correctly? |
|
@cgewecke this example works for me aswell. It seems like .slice(1) on the topics array did the trick and solved my issue aswell. While versions before 1.2.8 worked without slicing it, newer don't, so code that is written this way needs to be updated! Thanks for helping me resolving the issue! |
To be clear: the old Ethers v4 ABICoder did not have this problem, since it was the one used in I'd love @ricmoo's opinion on this as well, but since it's a somewhat prevalent solidity bug (lots of contracts were compiled with 0.4, especially older proxies), it is one of these weird edge cases to factor in when building and testing an ABI encoder. |
|
The old coder didn’t have a problem with it because of a bug. :p For context, that was a very old version of the ABI coder, which was fixed and that fix got brought along into v5. :) If that fix had been around sooner, the bug would have hopefully been caught before any contract went to production. Alas, my bad. :( The new error recovery API also cannot handle this, since it is a buffer overrun and not just a data type issue. One solution would be to sanitize the data before feeding it to the abi coder, but this could also break the revert reason, which is also important. Can we get an idea of how many contracts and their popularity are impacted? There isn’t really a good/clean/obvious fix, since it is literally wrong data being emitted from the contract. |
Lacking context, but would it be naive to only do this sanitization when decoding logs (since I believe the bug may only be apparent there)?
I haven't done the homework (not sure if there's a good automated way of detecting this), but not being able to handle historic event data is my main concern. If I'm correct, it looks like the bug is present only in these conditions:
While we (Aragon) are likely in the minority as our users still use 0.4.24 contracts and we haven't migrated them to 0.5+, this problem would still exist when we fetch old event data from contracts that were affected. |
|
Update: discussion about this is proceeding at ethers 891. For the moment it doesn't look like there's a simple fix (e.g: right padding) we can make here that works for events with more than one dynamically sized data type. |
cgewecke
changed the title
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions |
|
I've added support in ethers 5.0.12 for supporting legacy Solidity 0.4 external event data. I believe web3 depends on the abi package directly, in which case web3 should upgrade to For more details, please see ethers-io/ethers.js#891. |
Expected behavior
Using
decodeLogsshould parse logs correctly, see here an exampleActual behavior
Throws with an error like this if there’s a
bytesfield in the log:Steps to reproduce the behavior
Call that function mentioned above with a log like this one:
Logs
Copied above.
Environment
npm: 6.13.4
Node: v10.15.0
web3: 1.2.8
OS: PureOS 9.0
Reference
Related issue: aragon/contract-helpers#32
The text was updated successfully, but these errors were encountered: