-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Pass raw data with ContractLogicError #2922
Pass raw data with ContractLogicError #2922
Conversation
1c68bb0
to
4c780d7
Compare
4c780d7
to
cd2f9ce
Compare
3a8121a
to
2985a6a
Compare
2985a6a
to
3629fba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I like this approach in general! The more information the better on exceptions 👌🏼.
I think we should be able to add this to the OffchainLookup
exception also which would be some nice information to have available.
offchain_lookup_contract.functions.testOffchainLookup( | ||
OFFCHAIN_LOOKUP_TEST_DATA | ||
).call(ccip_read_enabled=False) | ||
assert e.value.data == OFFCHAIN_LOOKUP_RETURN_DATA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fselmo this assertion fails if it's run in isolation. the OFFCHAIN_LOOKUP_RETURN_DATA
changes, but only fails that way on eth-tester, not with the geth integration tests. Do you have any ideas why the OFFCHAIN_LOOKUP_RETURN_DATA
would be different when the whole file is run vs. run in isolation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah. The very first value that's in the OffchainLookup
revert in the contract is the particular contract's address which will differ between the contract deployed via the geth fixture and the one for eth-tester. You'll just have to change the beginning of the constant value for OFFCHAIN_LOOKUP_RETURN_DATA
for the eth-tester tests to match that.
It should only differ in that and I checked that:
>>> e.value.data[:10] + e.value.data[74:] == OFFCHAIN_LOOKUP_RETURN_DATA[:10] + OFFCHAIN_LOOKUP_RETURN_DATA[74:]
True
first 10: "0x" (2 indexes) + 4 byte function selector (8 indexes)
next 64 should be abi-encoded address value (32 bytes * 2 indexes per byte)
[74:] is the rest
so the differing 32 bytes should be indexes [10:74] in the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, thank you! I'll update the assertion so that it only asserts on e.value.data[:74]
or at least skips the middle address section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or just update [10:74] to match the address that deploys to the eth-tester
fixture
99a5996
to
7d30587
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
825abfc
to
5e451db
Compare
5e451db
to
6049c73
Compare
What was wrong?
People frequently request the raw data to be sent back with the ContractLogicError.
Closes #2793, #2336
How was it fixed?
This PR sends back the raw data as an attribute on the error class without breaking the existing API.
Todo:
Cute Animal Picture