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

Implementation of toEthAddress function #15

Closed
wants to merge 7 commits into from
Closed

Implementation of toEthAddress function #15

wants to merge 7 commits into from

Conversation

vaniiiii
Copy link

@vaniiiii vaniiiii commented Oct 25, 2023

Description

This pull request adds the required functionality related to issue #4

Developers can utilize this function to convert a f4 FileAddress type into an EthAddress.

Approach and implementation

The starting point was PR #16 which already included the implementation of this function. However, unit tests for various types of addresses were missing. The missing unit tests are added while retaining the existing fuzz tests with minor changes.

Validate function changes

The validate function checks the FilAddress in bytes format, which includes a protocol byte and payload.

The previous implementation was failing for valid input because it was comparing the bytes length with 10:

if (addr.data[0] == 0x00) {
    return addr.data.length <= 10; // should be <= 11 
}

This approach is incorrect, as the f0 format contains a protocol byte and payload, which can be a maximum of 10 bytes long. An example is provided in the Filecoin spec document, in the last example in section 6.1.3.1.

|----------|---------------|
| protocol |    payload    |
|----------|---------------|
|    0     | leb128-varint |

Discussion

  • Should the validate function also include checks for minimum length for f0 and f4 addresses? For example, should 0x00 be considered a valid f0 address?
  • Should f4 addresses be checked against a length of 65 if the payload is hardcoded to 54, as specified in FIP. If the payload is not 64 - prefix.len(), the total length, according to the format, should be 65 bytes. More details can be found in this discussion
|------------|----------------|---------------------|
|  protocol  |      actor     |      payload        | 
|------------|----------------|---------------------|
|     04     |  leb128-variant|      54 bytes       |

@vaniiiii vaniiiii added the enhancement New feature or request label Oct 25, 2023
@vaniiiii vaniiiii added this to the Milestone 2 milestone Oct 25, 2023
@vaniiiii vaniiiii self-assigned this Oct 25, 2023
@vaniiiii vaniiiii force-pushed the issue/#4 branch 2 times, most recently from e3a7107 to 575cc59 Compare November 3, 2023 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants