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

Use special strings for methodSig to handle contract call by pre-compiled ABI data #123

Merged
merged 5 commits into from
Jul 19, 2024

Conversation

songge-cb
Copy link
Contributor

@songge-cb songge-cb commented Jul 9, 2024

Motivation

This PR is to add some logic to the preprocess API to handle fallback pattern contract call. This type of contract call does not have a method signature.

Solution

We will use either empty string or a unique string "NO-METHOD-SIG" to indicate it. The pre-compiled ABI data will be passed in as methodArgs, either in the string format or the first element of an array.

Testing

  • Tested Preprocess API locally by passing empty string and "NO-METHOD-SIG" as methodSig.

Open questions

@songge-cb songge-cb marked this pull request as ready for review July 9, 2024 18:30
@songge-cb songge-cb force-pushed the songge/specialMethodSig branch 2 times, most recently from 846d6e6 to 71e716f Compare July 18, 2024 05:13
@songge-cb songge-cb changed the title Handle method passed in as hexdecimal string Use special strings for methodSig to handle contract call by pre-compiled ABI data Jul 19, 2024
return argStr, nil
}
return methodArgs, nil
case []string:
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to handle this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this specific project, no. Doing it for generalization.

}

switch args := methodArgs.(type) {
case []interface{}:
Copy link
Contributor

Choose a reason for hiding this comment

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

Will calling fallback ever have more than 1 args?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know, no. The arg is the pre-compiled data, which is one piece.

// preprocessArgs converts methodArgs to a string value if methodSig is an empty string.
// We are calling a contract written with fallback pattern, which has no method signature.
func preprocessArgs(methodSig string, methodArgs interface{}) (interface{}, error) {
if methodSig != "" && methodSig != NoMethodSig {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's simplify your impl like this:

func preprocessArgs(methodSig string, methodArgs interface{}) (interface{}, error) {
	if methodSig == "" || methodSig == NoMethodSig {
		switch args := methodArgs.(type) {
		case []interface{}:
			if len(args) == 1 {
				if argStr, ok := args[0].(string); ok {
					return argStr, nil
				}
				return nil, fmt.Errorf("failed to convert method arg \"%T\" to string", args[0])
			}
		case []string:
			if len(args) == 1 {
				return args[0], nil
			}
		}
	}

	return methodArgs, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR.

@GeekArthur GeekArthur merged commit 0e198c3 into coinbase:master Jul 19, 2024
8 checks passed
@cb-heimdall
Copy link

cb-heimdall commented Jul 26, 2024

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants