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

Implement utility traits for tendermint data types #64

Merged
merged 1 commit into from
Nov 22, 2019

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Nov 12, 2019

I removed the Default instances that don't make sense.

@yihuang yihuang force-pushed the implement-default-traits branch 3 times, most recently from 17ce65f to 5149dfb Compare November 12, 2019 04:21
tendermint/src/account.rs Outdated Show resolved Hide resolved
tendermint/src/public_key.rs Outdated Show resolved Hide resolved
tendermint/src/time.rs Outdated Show resolved Hide resolved
@yihuang
Copy link
Contributor Author

yihuang commented Nov 12, 2019

  • Removed Default instance for Account::Id, PublicKey, validator::Info (I must have missed that).
  • Fixed Time format to use .to_rfc3339_opts(SecondsFormat::Micros, true)

Copy link
Contributor

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

I don't think an all-zero hash or a default time makes sense. The rest looks fine.

tendermint/src/time.rs Outdated Show resolved Hide resolved
tarcieri
tarcieri previously approved these changes Nov 14, 2019
@yihuang
Copy link
Contributor Author

yihuang commented Nov 20, 2019

I've changed the format of Time to SecondsFormat::Nanos, according to tendermint implementation.

Copy link
Contributor

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

Perhaps I'm a bit jaded by (in some cases security) vulnerabilities relating to Go zero values, which it seems is how Default is being leveraged in the cases I pointed out in this review.

If you're looking for a way to easily construct test fixtures, I think Default is the wrong tool for the job. Alternatively, in the test suite itself, you can define a trait specifically for test fixtures in the test suite itself, and then impl it however you want!

trait TestFixture {
    fn test_fixture() -> Self;
}

impl TestFixture for Hash {
    fn test_fixture() -> Hash {
        Hash::Null
    }
}

In general, I think it's a bad idea for anything that's a testing-related convenience to leak outside of the test context, especially when it could potentially lead to security issues in production.

Is that what most of these are for?

tendermint/src/block/header.rs Outdated Show resolved Hide resolved
tendermint/src/block/id.rs Show resolved Hide resolved
tendermint/src/hash.rs Show resolved Hide resolved
tendermint/src/node/info.rs Outdated Show resolved Hide resolved
tendermint/src/version.rs Outdated Show resolved Hide resolved
@yihuang yihuang force-pushed the implement-default-traits branch 3 times, most recently from 9d903e4 to bf6f276 Compare November 21, 2019 11:14
@yihuang
Copy link
Contributor Author

yihuang commented Nov 21, 2019

Perhaps I'm a bit jaded by (in some cases security) vulnerabilities relating to Go zero values, which it seems is how Default is being leveraged in the cases I pointed out in this review.

If you're looking for a way to easily construct test fixtures, I think Default is the wrong tool for the job. Alternatively, in the test suite itself, you can define a trait specifically for test fixtures in the test suite itself, and then impl it however you want!

trait TestFixture {
    fn test_fixture() -> Self;
}

impl TestFixture for Hash {
    fn test_fixture() -> Hash {
        Hash::Null
    }
}

In general, I think it's a bad idea for anything that's a testing-related convenience to leak outside of the test context, especially when it could potentially lead to security issues in production.

Is that what most of these are for?

Yeah, actually we've changed the test code to use more properly constructed mocking data now.
But what's left in this patch now IMO are quite reasonable ;D

tarcieri
tarcieri previously approved these changes Nov 21, 2019
@tarcieri
Copy link
Contributor

@yihuang looks like this needs a GPG signature

@yihuang
Copy link
Contributor Author

yihuang commented Nov 22, 2019

@yihuang looks like this needs a GPG signature

Done.

@tarcieri tarcieri merged commit e39f2d5 into informalsystems:master Nov 22, 2019
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.

2 participants