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

Remove Default implementation for AccountId #1255

Merged
merged 14 commits into from
Feb 1, 2023

Conversation

HCastano
Copy link
Contributor

@HCastano HCastano commented May 16, 2022

The problem here is that in the sr25519 and ed25519 curves the
zero-address public key has a known private key.

The Default implementation of AccountId returned the zero-address. Since developers
frequently reach for Default it is entirely possible that they could shoot themselves
in the foot unknowingly. For example, assigning a contract owner to the zero address
and not updating it, or using it as a burn address.

There's also an argument to be made that the concept of the Default address doesn't
make sense anyways, and we shouldn't be giving this specific address a special treatment.

Closes #1110.

@HCastano HCastano added the E-on-ice The issue or PR is currently on ice and not further discussed or developed. label Jun 14, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2023

Codecov Report

Merging #1255 (b7feae2) into master (3cec278) will decrease coverage by 5.66%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1255      +/-   ##
==========================================
- Coverage   70.38%   64.73%   -5.66%     
==========================================
  Files         207      207              
  Lines        6393     6381      -12     
==========================================
- Hits         4500     4131     -369     
- Misses       1893     2250     +357     
Impacted Files Coverage Δ
crates/env/src/call/call_builder.rs 0.00% <0.00%> (ø)
crates/env/src/types.rs 0.00% <ø> (ø)
crates/primitives/src/types.rs 20.00% <ø> (ø)
crates/ink/ir/src/ir/contract.rs 0.00% <0.00%> (-91.67%) ⬇️
crates/ink/ir/src/ir/ink_test.rs 0.00% <0.00%> (-87.50%) ⬇️
crates/ink/ir/src/ir/trait_def/mod.rs 0.00% <0.00%> (-81.82%) ⬇️
crates/ink/ir/src/ir/storage_item/mod.rs 0.00% <0.00%> (-72.92%) ⬇️
crates/ink/ir/src/ir/storage_item/config.rs 0.00% <0.00%> (-68.75%) ⬇️
crates/ink/ir/src/ir/blake2.rs 20.00% <0.00%> (-60.00%) ⬇️
crates/ink/ir/src/ir/trait_def/config.rs 7.40% <0.00%> (-55.56%) ⬇️
... and 20 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@HCastano HCastano removed the E-on-ice The issue or PR is currently on ice and not further discussed or developed. label Feb 1, 2023
@HCastano HCastano marked this pull request as ready for review February 1, 2023 04:26
crates/env/src/call/call_builder.rs Outdated Show resolved Hide resolved
crates/env/src/engine/on_chain/impls.rs Outdated Show resolved Hide resolved
* Make cross-contract callee non-optional

* clippy

* Fmt

* Fix

* clippy

* clippy

* clippy

* Add a similar method for `code_hash`

* Fix doc tests

* RustFmt

* Rename top level methods to `call` and `delegate`

* Fix some renames

---------

Co-authored-by: Hernando Castano <hernando@hcastano.com>
@ascjones ascjones merged commit ce64dbe into master Feb 1, 2023
@ascjones ascjones deleted the hc-remove-default-account-id branch February 1, 2023 18:53
@ascjones ascjones mentioned this pull request Feb 15, 2023
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.

Double check usage of Default AccountIds
3 participants