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: support evm address #668

Merged
merged 21 commits into from
Nov 9, 2022
Merged

feat: support evm address #668

merged 21 commits into from
Nov 9, 2022

Conversation

MujkicA
Copy link
Contributor

@MujkicA MujkicA commented Nov 2, 2022

Closes #661

The EvmAddress is implemented equivalent to the Sway type, as a wrapper over Bits256. And just like in Sway, the first 12 bytes are zeroed out when it is instantiated from Bits256.
Encoding/decoding this type is treated as a struct with a single Bits256 field.

@MujkicA MujkicA added the enhancement New feature or request label Nov 2, 2022
@MujkicA MujkicA self-assigned this Nov 2, 2022
@MujkicA MujkicA added the blocked label Nov 2, 2022
@MujkicA
Copy link
Contributor Author

MujkicA commented Nov 2, 2022

Needs forc update

@MujkicA MujkicA removed the blocked label Nov 4, 2022
@MujkicA MujkicA marked this pull request as ready for review November 4, 2022 11:01
// ANCHOR_END: evm_address

impl EvmAddress {
fn clear_12_bytes(bytes: [u8; 32]) -> [u8; 32] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's private might save space and just make it a fn instead of an associated fn.

Goes without saying that it doesn't make any difference functionally.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like that it's an associated function since it's a very specific "utility", it would be weird to see it implemented outside this context.

packages/fuels-core/src/types/bits_256.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@iqdecay iqdecay left a comment

Choose a reason for hiding this comment

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

Looking good, some questions

packages/fuels-core/src/types/bits_256.rs Show resolved Hide resolved
// ANCHOR_END: evm_address

impl EvmAddress {
fn clear_12_bytes(bytes: [u8; 32]) -> [u8; 32] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that it's an associated function since it's a very specific "utility", it would be weird to see it implemented outside this context.

packages/fuels-core/src/types/bits_256.rs Show resolved Hide resolved
packages/fuels-core/src/types/bits_256.rs Outdated Show resolved Hide resolved
packages/fuels/tests/bindings.rs Show resolved Hide resolved
@MujkicA MujkicA requested a review from iqdecay November 7, 2022 10:24
iqdecay
iqdecay previously requested changes Nov 7, 2022
Copy link
Contributor

@iqdecay iqdecay 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 we should make it clearer if we're going with the leftmost clearing just by convention or specs. Otherwise great!

docs/src/types/evm_address.md Outdated Show resolved Hide resolved
{{#include ../../../packages/fuels/tests/bindings.rs:evm_address_arg}}
```

> **Note:** when creating an `EvmAddress` from `Bits256`, the first 12 bytes will be cleared because an evm address is only 20 bytes long.
Copy link
Contributor

Choose a reason for hiding this comment

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

Explain the choice of clearing the leftmost ones. If it is a deliberate choice (ie not based on spec), then this choice should be stated clearly ("We chose the convention of clearing the leftmost bytes").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The choice was made based on the Sway implementation of the same type. I find this a bit tricky to comment since we had a task to specifically remove all mentions of sway.

packages/fuels-core/src/types/bits_256.rs Show resolved Hide resolved
Co-authored-by: iqdecay <victor@berasconsulting.com>
hal3e
hal3e previously approved these changes Nov 8, 2022
Copy link
Contributor

@hal3e hal3e left a comment

Choose a reason for hiding this comment

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

LGTM. Good work! Left just a tiny comment.

packages/fuels-core/src/types/bits_256.rs Outdated Show resolved Hide resolved
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

Very nice!

@MujkicA MujkicA merged commit e1a68d4 into master Nov 9, 2022
@MujkicA MujkicA deleted the mujkica/support-evm-address branch November 9, 2022 10:36
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.

Add support for the EvmAddress type
5 participants