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

feat: allow all unrestricted method numbers on accounts #1086

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Jan 20, 2023

This patch implements a "raw" fallback for both account types that simply accepts all methods greater than or equal to the min FRC0042 number.

Alternative to #1081

Fixes filecoin-project/ref-fvm#1518

@Stebalien Stebalien requested review from arajasek and anorth and removed request for arajasek January 20, 2023 20:01
@arajasek
Copy link
Contributor

The approach looks good to me, thank you!

I think the macros look good, but I'm honestly not especially sure.

@Stebalien Stebalien force-pushed the steb/account-any-unrestricted branch 2 times, most recently from d0f2072 to 18b95d5 Compare January 20, 2023 20:59
@Stebalien
Copy link
Member Author

I think the macros look good, but I'm honestly not especially sure.

The best way to review macros is to try expanding them. E.g.:

fn invoke_method<RT>(
    rt: &mut RT,
    method: MethodNum,
    args: Option<fvm_ipld_encoding::ipld_block::IpldBlock>,
) -> Result<Option<fvm_ipld_encoding::ipld_block::IpldBlock>, ActorError>
where
    RT: Runtime,
    RT::Blockstore: Clone,
{
    restrict_internal_api(rt, method)?;
    match FromPrimitive::from_u64(method) {
        Some(Self::Methods::Constructor) => {
            $crate::dispatch(rt, Self::constructor, &args)
        }
        Some(Self::Methods::AuthenticateMessageExported) => {
            $crate::dispatch(rt, Self::authenticate_message, &args)
        }
        None => Self::fallback(rt, method, args),
        None => Err($crate::ActorError::unhandled_message({
            let res = $crate::fmt::format(std::fmt::Arguments::new_v1(
                &[],
                &[std::fmt::ArgumentV1::new(&(method), std::fmt::Display::fmt)],
            ));
            res
        })),
    }
}

@Stebalien Stebalien force-pushed the steb/account-any-unrestricted branch from 18b95d5 to fc736c3 Compare January 20, 2023 23:30
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

I think something weird happened to the PR and I can't see any macro changes. The raw macro thing isn't on master.

  1. I think this should accept all unimplemented methods. I don't think FRC-0042 should be privileged here. We left a large range below that because there could be good call for other schemes.
  2. I don't think we should use the macros in these actors. The additional conceptual complexity and indirection isn't worth it. It would be better to be blindingly obvious how it works.

@Stebalien
Copy link
Member Author

I think something weird happened to the PR and I can't see any macro changes. The raw macro thing isn't on master.

#1088

I think this should accept all unimplemented methods. I don't think FRC-0042 should be privileged here. We left a large range below that because there could be good call for other schemes.

What if we need to add new methods in this low method range?

@arajasek
Copy link
Contributor

  1. I think this should accept all unimplemented methods. I don't think FRC-0042 should be privileged here. We left a large range below that because there could be good call for other schemes.

Yeah, I'm not sure I like this idea -- we likely need some bound here, and I think the FRC-0042 range makes sense. I'm comfortable with that level of "privileging" FRC-0042.

Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

This LGTM.

@anorth
Copy link
Member

anorth commented Jan 23, 2023

Ok on the FRC-0042 thing: filecoin-project/FIPs#592 (reply in thread)

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.

Implement FRC0042 InvokeEVM on Account & EthAccount
3 participants