Skip to content

Deleted update functions, getSingleNodeRootHash internal functions, a…#2389

Merged
mergify[bot] merged 2 commits intoethereum-optimism:developfrom
Michael-Vander-Meiden:iss2377
Apr 3, 2022
Merged

Deleted update functions, getSingleNodeRootHash internal functions, a…#2389
mergify[bot] merged 2 commits intoethereum-optimism:developfrom
Michael-Vander-Meiden:iss2377

Conversation

@Michael-Vander-Meiden
Copy link
Contributor

@Michael-Vander-Meiden Michael-Vander-Meiden commented Mar 31, 2022

Description
packages/contracts/contracts/libraries/trie/Lib_MerkleTrie.sol:

  • deleted update function
  • deleted getSingleNodeRootHash internal function

packages/contracts/contracts/libraries/trie/Lib_SecureMerkleTrie.sol:

  • deleted update function
  • deleted getSingleNodeRootHash internal function

packages/contracts/test/contracts/libraries/trie/Lib_MerkleTrie.spec.ts

  • deleted test for update function

packages/contracts/test/contracts/libraries/trie/Lib_SecureMerkleTrie.spec.ts

  • deleted test for update function
  • deleted test for getSingleNodeRootHash internal function

Metadata

@changeset-bot
Copy link

changeset-bot bot commented Mar 31, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 1867a4c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@eth-optimism/contracts Patch
@eth-optimism/data-transport-layer Patch
@eth-optimism/sdk Patch
@eth-optimism/message-relayer Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mergify mergify bot requested review from mslipper and tuxcanfly March 31, 2022 20:34
@tynes
Copy link
Contributor

tynes commented Mar 31, 2022

Can you add a changeset for the contracts package and squash into a single commit? Thanks!

@Michael-Vander-Meiden
Copy link
Contributor Author

Michael-Vander-Meiden commented Mar 31, 2022

Sure thing, will do that now

@Michael-Vander-Meiden
Copy link
Contributor Author

Michael-Vander-Meiden commented Mar 31, 2022

@tynes done!

@tynes
Copy link
Contributor

tynes commented Apr 1, 2022

Thanks for updating @Michael-Vander-Meiden! I apologize and should have mentioned that this should be a patch changeset. Pre 1.0.0 we aren't following semver exactly, do you mind updating this? Then it will be good to go

@Michael-Vander-Meiden
Copy link
Contributor Author

Michael-Vander-Meiden commented Apr 1, 2022

Thanks for updating @Michael-Vander-Meiden! I apologize and should have mentioned that this should be a patch changeset. Pre 1.0.0 we aren't following semver exactly, do you mind updating this? Then it will be good to go

Sorry I just saw this. Third time's the charm! Switched it to patch. 😁

@tynes tynes requested a review from maurelian April 1, 2022 16:49
@tynes
Copy link
Contributor

tynes commented Apr 1, 2022

cc @maurelian

maurelian
maurelian approved these changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@maurelian maurelian left a comment

Choose a reason for hiding this comment

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

This is safe to merge once the checks are running, but it would be preferable to also delete _getUpdatedTrieRoot().
If we can get that in quickly, great. But I'll approve regardless.

@mslipper
Copy link
Collaborator

mslipper commented Apr 1, 2022

Hi Michael!

To get builds running please unfollow your fork of optimism and follow the repo under our organization. CircleCI uses which repo you follow to determine which repo to build. More details from CircleCI's documentation:

Note: If a user submits a pull request to your repository from a fork, but no pipeline is triggered, then the user most likely is following a project fork on their personal account rather than the project itself of CircleCi, causing the jobs to trigger under the user’s personal account and not the organization account. To resolve this issue, have the user unfollow their fork of the project on CircleCI and instead follow the source project. This will trigger their jobs to run under the organization when they submit pull requests.

@maurelian
Copy link
Contributor

maurelian commented Apr 2, 2022

Ugh, sorry for the trouble @Michael-Vander-Meiden, but you'll need to rebase this on develop, and run yarn lint to fix that failed check.

Thanks for working through this with us.

@Michael-Vander-Meiden
Copy link
Contributor Author

Michael-Vander-Meiden commented Apr 2, 2022

No worries, working on it now.

@Michael-Vander-Meiden
Copy link
Contributor Author

Michael-Vander-Meiden commented Apr 2, 2022 •
edited
Loading

hey @maurelian , just want to make sure I get this right. When you say rebase on develop, do you mean pull develop into my iss2377 branch and then lint, or something else?

edit
feeling kind of dumb for the question but I should just go to my branch, git rebase develop and thats that?

@maurelian
Copy link
Contributor

maurelian commented Apr 2, 2022

git pull --rebase origin develop should be the command you need. The important thing is that your changes are made on top of the latest commit on our develop.

@Michael-Vander-Meiden Michael-Vander-Meiden force-pushed the iss2377 branch 2 times, most recently from b43b7dc to 0614825 Compare April 2, 2022 18:27
@Michael-Vander-Meiden
Copy link
Contributor Author

Michael-Vander-Meiden commented Apr 2, 2022

Hey @maurelian ! There ended up being a lot more unused functions than I originally thought. I deleted those as well some more tests and fixed the linting issues.

If things still look good to you then I will go ahead and squash these commits to prep for merge.

@maurelian
Copy link
Contributor

maurelian commented Apr 2, 2022

Nothing like the smell of deleted code in the morning!These changes look great, squash away.

@Michael-Vander-Meiden
Copy link
Contributor Author

Michael-Vander-Meiden commented Apr 2, 2022

@maurelian looks ready for merge!😁

@mergify
Copy link
Contributor

mergify bot commented Apr 3, 2022

This PR has been added to the merge queue, and will be merged soon.

@mergify mergify bot merged commit d040a8d into ethereum-optimism:develop Apr 3, 2022
@mergify
Copy link
Contributor

mergify bot commented Apr 3, 2022

This PR is next in line to be merged, and will be merged as soon as checks pass.

nebojsa94 pushed a commit to Tenderly/optimism that referenced this pull request Apr 26, 2022
@Michael-Vander-Meiden @maurelian @nebojsa94
Deleted update functions, helper internal functions, and tests. added…
2fe033f 
… changeset (ethereum-optimism#2389)

Co-authored-by: Maurelian <maurelian@protonmail.ch>
theochap added a commit that referenced this pull request Dec 10, 2025
@theochap
chore(node/engine): cleanup engine tasks errors. refactors the engine…
76fd73c 
… build task to reuse the insert task (#2399)

## Description

This PR does the following cleanups to the engine tasks:

- Use explicit error types instead of `dyn Box<...>` for the
`EngineTaskError`. The goal of this is twofold:
- Improve the composability of engine tasks (and be able to do some
control flow when an engine task emits an error)
- Allows explicit testing of the error cases (in the future when we'll
add tests for the engine tasks)
- Refactors the build task in the engine to reuse the `InsertUnsafe`
task logic. This task is now called `InsertTask` to also encapsulate the
case where we insert a derived block.

Close #2390. Progress towards #2389
theochap added a commit that referenced this pull request Jan 14, 2026
@theochap
chore(node/engine): cleanup engine tasks errors. refactors the engine…
e1a3d78 
… build task to reuse the insert task (op-rs/kona#2399)

## Description

This PR does the following cleanups to the engine tasks:

- Use explicit error types instead of `dyn Box<...>` for the
`EngineTaskError`. The goal of this is twofold:
- Improve the composability of engine tasks (and be able to do some
control flow when an engine task emits an error)
- Allows explicit testing of the error cases (in the future when we'll
add tests for the engine tasks)
- Refactors the build task in the engine to reuse the `InsertUnsafe`
task logic. This task is now called `InsertTask` to also encapsulate the
case where we insert a derived block.

Close #2390. Progress towards #2389
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mslipper mslipper mslipper approved these changes

@maurelian maurelian maurelian approved these changes

@tuxcanfly tuxcanfly Awaiting requested review from tuxcanfly

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Delete update code from Lib_MerkleTrie

4 participants

@Michael-Vander-Meiden @tynes @mslipper @maurelian