Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

stargate: migrate types #670

Merged
merged 16 commits into from
Jan 6, 2021
Merged

stargate: migrate types #670

merged 16 commits into from
Jan 6, 2021

Conversation

fedekunze
Copy link
Contributor

@fedekunze fedekunze commented Dec 23, 2020

Description

Manual split of changes from #546. This PR updates struct types to the ones that will be defined on the proto files


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@fedekunze fedekunze added the x/evm EVM module issues label Dec 23, 2020
@fedekunze fedekunze marked this pull request as ready for review December 24, 2020 14:34
@fedekunze fedekunze requested a review from noot as a code owner December 24, 2020 14:34
x/evm/keeper/keeper.go Outdated Show resolved Hide resolved
x/evm/keeper/keeper.go Outdated Show resolved Hide resolved
x/evm/genesis.go Show resolved Hide resolved
return fmt.Errorf("address cannot be the zero address %s", ga.Address)
}
if ga.Code != nil && len(ga.Code) == 0 {
return errors.New("code bytes cannot be empty")
if ga.Code == "" || len(ethcmn.Hex2Bytes(ga.Code)) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

cases seem redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which one?

Copy link
Contributor

Choose a reason for hiding this comment

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

either one, I think if one is satisfied then the other always is

Comment on lines +8 to +9
type Recipient struct {
Address string
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this no longer an address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

protobuf

AccountNonce uint64
Price *big.Int `json:"gasPrice"`
GasLimit uint64 `json:"gas"`
Recipient *ethcmn.Address `json:"to" rlp:"nil"` // nil means contract creation
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just use *ethcmn.Address as the recipient type in the msg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, because of the proto types we need to use string, and you can't take a string pointer on protobuf, so we need to wrap it on this Recipient struct

Comment on lines +282 to +284
V *big.Int `json:"v"`
R *big.Int `json:"r"`
S *big.Int `json:"s"`
Copy link
Contributor

Choose a reason for hiding this comment

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

same for these, why are these now []byte in the msg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's no big.Int on protobuf

x/evm/types/storage.go Show resolved Hide resolved
@fedekunze fedekunze merged commit 9cbb4dc into development Jan 6, 2021
@fedekunze fedekunze deleted the stargate-types branch January 6, 2021 20:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants