Skip to content

refactor(eth): attach ToFilecoinMessage converter to EthCall type#12062

Closed
raulk wants to merge 2 commits intomasterfrom
raulk/refactor-eth-message
Closed

refactor(eth): attach ToFilecoinMessage converter to EthCall type#12062
raulk wants to merge 2 commits intomasterfrom
raulk/refactor-eth-message

Conversation

@raulk
Copy link
Copy Markdown
Member

@raulk raulk commented May 31, 2024

Small refactor to clean things up.

  • This converter is better placed as an EthCall method instead of a package-level function.
  • The resulting package/module import graph is sane.
  • Exporting this as a public method is very helpful for those who use Lotus as a library.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 31, 2024

All checks have passed

@raulk raulk force-pushed the raulk/refactor-eth-message branch from c538885 to 3d98bd0 Compare May 31, 2024 18:06
@jennijuju jennijuju requested a review from aarshkshah1992 May 31, 2024 20:07
@rjan90
Copy link
Copy Markdown
Contributor

rjan90 commented Jun 3, 2024

Thanks for the PR! Aarsh will do a pass on it when he is back (he is OOO today).

In the meantime, it looks like you need to run: make gen / make docsgen-cli for the two last checks there to pass.

@Stebalien
Copy link
Copy Markdown
Member

Er, sorry, I didn't see the comment from @rjan90.

Make gen / Make docsgen-cli
@rjan90
Copy link
Copy Markdown
Contributor

rjan90 commented Jun 3, 2024

Pushed make gen / make doscgen-cli, to unblock this PR (hope that was okay Raul) so that we can merge it.

@Stebalien
Copy link
Copy Markdown
Member

Test failure looks real.

@raulk
Copy link
Copy Markdown
Member Author

raulk commented Jun 4, 2024

@rjan90 thanks for running make! ❤️
@Stebalien -- yep, it looks real. I'm investigating.

@rjan90
Copy link
Copy Markdown
Contributor

rjan90 commented Jan 23, 2025

Hi @raulk, I'm considering whether it's still worthwhile to finalize this change, or if this PR should get moved back to draft/be closed. I can't recall the specific test that was failing, but it appears to have been resolved, as all checks are currently passing. What do you think?

@rvagg
Copy link
Copy Markdown
Member

rvagg commented Jan 23, 2025

👍 this would be a good change; unfortunately things have moved around a bit, the original function being refactored is now in node/impl/eth/utils.go, so this would need a little bit of work.

@rjan90
Copy link
Copy Markdown
Contributor

rjan90 commented Jan 23, 2025

I've open a PR with the changes here: #12844. Starting from the master branch seemed to be the simplest approach.

@rjan90
Copy link
Copy Markdown
Contributor

rjan90 commented Jan 24, 2025

Superseded and fixed by: #12844

@rjan90 rjan90 closed this Jan 24, 2025
@rjan90 rjan90 deleted the raulk/refactor-eth-message branch January 30, 2025 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ☑️ Done (Archive)

Development

Successfully merging this pull request may close these issues.

4 participants