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.

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

test: StateAccount.Extra via trie.StateTrie.{Update,Get}Account() #45

Merged
merged 4 commits into from
Oct 2, 2024

Conversation

ARR4N
Copy link
Collaborator

@ARR4N ARR4N commented Oct 2, 2024

Why this should be merged

Integration test of the new StateAccount.Extra payloads introduced in #44, originally with unit tests only.

How this works

trie.StateTrie round trip via UpdateAccount() and then GetAccount(), confirming that not only are the payloads correctly echoed but that they also modify the StateTrie's root hash as expected.

How this was tested

n/a

testonly.OrPanic(func() {})
}

eNil := e == nil || e.t == nil
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 was moved from the test file, unchanged.

)

// OrPanic runs `fn` i.f.f. called from within a testing environment.
func OrPanic(fn func()) {
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 was abstracted from params/config.libevm.go, unchanged except for the call to fn() and the size of the pc slice.

@ARR4N ARR4N marked this pull request as ready for review October 2, 2024 15:32
@ARR4N ARR4N requested review from a team, darioush, ceyonur and michaelkaplan13 and removed request for a team October 2, 2024 15:38
@ARR4N ARR4N enabled auto-merge (squash) October 2, 2024 15:38
@ARR4N ARR4N disabled auto-merge October 2, 2024 15:38
@ARR4N ARR4N enabled auto-merge (squash) October 2, 2024 15:38
Comment on lines +142 to +143
require.NoErrorf(t, state.UpdateAccount(addr, acct), "%T.UpdateAccount(...)", state)
assert.Equalf(t, tt.wantTrieHash, state.Hash(), "%T.Hash() after UpdateAccount()", state)
Copy link
Collaborator

@darioush darioush Oct 2, 2024

Choose a reason for hiding this comment

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

Can can we also add testing for Copy & Snapshot/Revert being handled properly?
For example statedb.Copy makes a shallow copy of the accounts modified, so we should ensure that setting the extras on the origin does not impact the copied statedb & the other way around.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Snapshot/Revert covered in #48

I'm about to test Copy too. If it fails then I'll see how much needs to change and might start a new PR if the fix spreads too far.

@ARR4N ARR4N merged commit 5ec080f into libevm Oct 2, 2024
3 checks passed
@ARR4N ARR4N deleted the arr4n/test-state-account-storage-retrieval branch October 2, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants