t8n: Fix deposit log and sys contract validations #1339
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@ ## master #1339 +/- ## ========================================== + Coverage 87.25% 87.27% +0.02% ========================================== Files 169 169 Lines 24976 25042 +66 Branches 4107 4118 +11 ========================================== + Hits 21792 21855 +63 - Misses 529 531 +2 - Partials 2655 2656 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
pdobacz force-pushed the deposit-logs branch 2 times, most recently from 838c40c to c223c3b Compare | requests.append({&log.data[AMOUNT_OFFSET], AMOUNT_SIZE}); | ||
| requests.append({&log.data[SIGNATURE_OFFSET], SIGNATURE_SIZE}); | ||
| requests.append({&log.data[INDEX_OFFSET], INDEX_SIZE}); | ||
| requests.append({&log.data[PUBKEY_OFFSET + WORD], PUBKEY_SIZE}); |
There was a problem hiding this comment.
note here, that since I needed to add validation and thus change the meaning of the variables, _OFFSET now points to the size word, not contents. It is in line with the EIP-6110 wording
chfast left a comment
chfast left a comment There was a problem hiding this comment.
Why don't we fail EEST currently?
Aah, great question, I was confused about this too at first! The block is rejected for another reason - request hashes don't match with expected. I'll try to add a test which would fail for us |
Actually, correction: I think it is not about the tests, but rather the filling client, what it does when it encounters invalid layout. It can either decide to put garbage into |
But tests are still failing to detect wrong implementation because the hash still mismatches but for different reason? |
Yes, I suppose the way to go would be to differentiate all the |
test/state/requests.cpp Outdated
| const auto size = read_word_as_size(offset); | ||
| return size.has_value() && (*size == expected_size); | ||
| }; | ||
| if (!validate_size(PUBKEY_OFFSET, PUBKEY_SIZE) || |
There was a problem hiding this comment.
I'm confused here. Are we validating the compile-time constants here? If yes, convert this to a static_assert.
There was a problem hiding this comment.
nope, the first argument is for lookup in the log.data. I'll improve the name to be more natural
| I've eyeballed the coverage report, all looks fine to me |
This means we don't have the coverage for the case where the logs are empty.  |
we do, just not PRed yet, sorry to skip over this :) and thank you for cross-checking |
This PR is for the fix anyway, so it doesn't matter here. |
Co-authored-by: Paweł Bylica <pawel@hepcolgum.band>
Closes #1337 . Requires a fix on EEST side in order to fill, I'll open a PR there after the Weld