Uh oh!
There was an error while loading. Please reload this page.
fix: reduce repetitive code in enum match blocks - #6453
Conversation
WalkthroughAdds workspace dependency spire_enum; applies its delegated_enum macro to several multi-version enums; introduces a new VM variant Changes
Sequence Diagram(s)(Skipped — changes are enum/delegation-focused and small control-flow tweaks; not a multi-component sequential flow.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| VM::VM3(fvm_executor) => Ok(fvm_executor.flush()?), | ||
| VM::VM4(fvm_executor) => Ok(fvm_executor.flush()?), | ||
| } | ||
| Ok(delegate_vm!(self.flush()?)) |
There was a problem hiding this comment.
to my understanding, we can avoid the declarative macro via #[delegate_impl], no?
There was a problem hiding this comment.
Tried #[delegate_impl] but it does not compile. According to the doc, #[delegate_impl] is used for trait impls.
https://docs.rs/spire_enum/1.0.0/spire_enum/index.html#2-delegate_impl-inherenttrait-impl-attribute-macro
There was a problem hiding this comment.
Hm, indeed. I wonder if VM should've been a trait. Something to consider in the future, but outside of the scope of this PR.
| State::V8(st) => st.address, | ||
| State::V9(st) => st.address, | ||
| State::V10(st) => from_address_v3_to_v2(st.address), | ||
| State::V11(st) => from_address_v3_to_v2(st.address), |
There was a problem hiding this comment.
How does it do it now? What's the error message in case of a fiasco?
There was a problem hiding this comment.
Sorry, I don't quite get it. This method has no error propogation.
There was a problem hiding this comment.
It doesn't, but it panics with a certain string.
pubfnfrom_address_v3_to_v2(addr:AddressV3) -> AddressV2{AddressV2::from_bytes(&addr.to_bytes()).expect("Couldn't convert between FVM3 and FVM2 addresses.")}Given the conversion is not trivial (one has to call from_bytes etc. and check if it worked), my question is what is generated under the hood.
There was a problem hiding this comment.
No conversion happens here, plz note that the fn return type has been changed to shim::Address. Previously, all addresses are converted to v2, then converted to shim::Addresshere
There was a problem hiding this comment.
Ah okay, I missed the signature change. LGTM.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.