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

update compiler version and remove deprecated entities #45

Merged
merged 2 commits into from
Dec 7, 2021

Conversation

nichonien
Copy link
Contributor

This PR contains changes :

  1. Upgrade compiler version
  2. Replaces now with block.timestamp
  3. Replaces byte with bytes1
  4. Updates keccak256 implementation to one parameter implementation (latest version)
  5. Updates 0x0 address to full length address

@mirceanis mirceanis self-requested a review November 30, 2021 19:57
@nichonien
Copy link
Contributor Author

@mirceanis Did you get a chance to look the at PR?

Copy link
Contributor

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Excellent work.

It took me a while to test this out since the existing test harness doesn't work on my machine any more.
We need to upgrade the test automation here as well if we want to keep improving the contract(s).

I made some suggestions regarding the way the message is hashed in the change/add/remove*Signed() methods, for maintaining compatibility with existing codebases.

Using abi.encodePacked instead of abi.encode allows the caller to use simple concatenation of byte arrays instead of doing RLP encoding of arguments.
Also, since there is at most one variable-length array parameter, the warning in the solidity docs does not apply.

contracts/EthereumDIDRegistry.sol Outdated Show resolved Hide resolved
contracts/EthereumDIDRegistry.sol Outdated Show resolved Hide resolved
contracts/EthereumDIDRegistry.sol Outdated Show resolved Hide resolved
contracts/EthereumDIDRegistry.sol Outdated Show resolved Hide resolved
contracts/EthereumDIDRegistry.sol Outdated Show resolved Hide resolved
contracts/EthereumDIDRegistry.sol Outdated Show resolved Hide resolved
@nichonien
Copy link
Contributor Author

Excellent work.

It took me a while to test this out since the existing test harness doesn't work on my machine any more. We need to upgrade the test automation here as well if we want to keep improving the contract(s).

I made some suggestions regarding the way the message is hashed in the change/add/remove*Signed() methods, for maintaining compatibility with existing codebases.

Using abi.encodePacked instead of abi.encode allows the caller to use simple concatenation of byte arrays instead of doing RLP encoding of arguments. Also, since there is at most one variable-length array parameter, the warning in the solidity docs does not apply.

Thanks @mirceanis for the feedback. I have made the suggested changes.

I do agree that the tests need to upgraded. It broke for me as well and I could only test it with our test-suite. I guess I could give sometime post next week to improve test cases.

@nichonien nichonien requested a review from mirceanis December 7, 2021 18:06
Copy link
Contributor

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Looks great!

Thank you for contributing

@mirceanis mirceanis merged commit c906383 into uport-project:develop Dec 7, 2021
@mirceanis
Copy link
Contributor

BTW, I have started a governance topic about did:ethr upgrades on our discord: https://discord.gg/MTeTAwSYe7

Feel free to join and share your thoughts.

uport-automation-bot pushed a commit that referenced this pull request Jul 7, 2022
# [1.0.0](v0.0.3...1.0.0) (2022-07-07)

### Bug Fixes

* update compiler version and remove deprecated entities ([#45](#45)) ([c906383](c906383))

* Merge pull request #53 from uport-project/develop ([210e1a5](210e1a5)), closes [#53](#53) [#12](#12) [#52](#52)

### Features

* add deployments JSON and use an index for the module ([ae9f393](ae9f393))
* adopt hardhat for development ([#46](#46)) ([0ab4f15](0ab4f15))

### BREAKING CHANGES

* Please see #12 and #52
uport-automation-bot pushed a commit that referenced this pull request Jul 7, 2022
# [1.0.0](v0.0.3...1.0.0) (2022-07-07)

### Bug Fixes

* update compiler version and remove deprecated entities ([#45](#45)) ([c906383](c906383))

* Merge pull request #53 from uport-project/develop ([210e1a5](210e1a5)), closes [#53](#53) [#12](#12) [#52](#52)

### Features

* add deployments JSON and use an index for the module ([ae9f393](ae9f393))
* adopt hardhat for development ([#46](#46)) ([0ab4f15](0ab4f15))

### BREAKING CHANGES

* Please see #12 and #52
@uport-automation-bot
Copy link

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants