Skip to content

Updates pubkey from bytes32 to bytes - #630

Merged
rauljordan merged 13 commits into
OffchainLabs:masterfrom
0xKiwi:hotfix/update-pubkey-to-bytes
Nov 6, 2018
Merged

Updates pubkey from bytes32 to bytes#630
rauljordan merged 13 commits into
OffchainLabs:masterfrom
0xKiwi:hotfix/update-pubkey-to-bytes

Conversation

@0xKiwi

@0xKiwi 0xKiwi commented Oct 8, 2018

Copy link
Copy Markdown
Contributor

Resolves issue #562


Changes pubkey from bytes32 to bytes

The changes made modify all the bytes32 to bytes and make sure its checks that it is a 48length byte array.
Also changes pubKey mapping to a hashPubKey mapping due to the changes required.

@terencechain terencechain left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good. Please add a test case in validator_registration_test.go

@codecov

codecov Bot commented Oct 8, 2018

Copy link
Copy Markdown

Codecov Report

Merging #630 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #630   +/-   ##
======================================
  Coverage    75.9%   75.9%           
======================================
  Files          56      56           
  Lines        3748    3748           
======================================
  Hits         2845    2845           
  Misses        648     648           
  Partials      255     255

@prestonvanloon

Copy link
Copy Markdown
Member

Don't you need to rebuild the go libraries?

// Deposit is a paid mutator transaction binding the contract method 0x881d2135.
//
// Solidity: function deposit(_pubkey bytes32, _withdrawalShardID uint256, _withdrawalAddressbytes32 address, _randaoCommitment bytes32) returns()
func (_ValidatorRegistration *ValidatorRegistrationTransactor) Deposit(opts *bind.TransactOpts, _pubkey [32]byte, _withdrawalShardID *big.Int, _withdrawalAddressbytes32 common.Address, _randaoCommitment [32]byte) (*types.Transaction, error) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Did you regenerate validator_registration.go or manually changed it [48]byte?

Weird it still says function deposit(_pubkey bytes32,... at the top

@0xKiwi

0xKiwi commented Oct 10, 2018

Copy link
Copy Markdown
Contributor Author

Changing the mapping from bytes32 to bytes is actually not possible in Solidity since bytes is a dynamic type. How should we approach this change for Solidity? Maybe use a hash of the bytes array?

@prestonvanloon

Copy link
Copy Markdown
Member

What is wrong with a dynamic type other than being wasteful?

https://solidity.readthedocs.io/en/v0.4.24/types.html#dynamically-sized-byte-array

Can you store the pubkey as two byte32s and concat them?

@0xKiwi

0xKiwi commented Oct 11, 2018

Copy link
Copy Markdown
Contributor Author

https://solidity.readthedocs.io/en/v0.4.25/types.html#mappings
Dynamic types can't be used as the key for mappings.
Mapping types are declared as mapping(_KeyType => _ValueType). Here _KeyType can be almost any type except for a mapping, a dynamically sized array, a contract, an enum and a struct. _ValueType can actually be any type, including mappings.

What would I concat them into though? I can't do a string (because dynamic length) or a number (48 bytes > 32 bytes).

@nisdas

nisdas commented Oct 20, 2018

Copy link
Copy Markdown
Contributor

@IvanTheGreatDev Whats the status on this ?

@0xKiwi

0xKiwi commented Oct 20, 2018 via email

Copy link
Copy Markdown
Contributor Author

@nisdas

nisdas commented Oct 20, 2018

Copy link
Copy Markdown
Contributor

@terenc3t @prestonvanloon What do you guys think ? Should we just store the hash of the pubkey instead in the mapping ?

@0xKiwi

0xKiwi commented Nov 5, 2018

Copy link
Copy Markdown
Contributor Author

Alright, I changed it to just use the hash of the bytes array the user passes in. Good for review now.

@terencechain terencechain left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

@CLAassistant

CLAassistant commented Nov 6, 2018

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@rauljordan
rauljordan merged commit b8eb258 into OffchainLabs:master Nov 6, 2018
@0xKiwi
0xKiwi deleted the hotfix/update-pubkey-to-bytes branch November 6, 2018 21:13
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.

6 participants