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

EthConnector: make AdminControlled and pausable #88

Merged
merged 13 commits into from
May 27, 2021

Conversation

sept-en
Copy link
Contributor

@sept-en sept-en commented May 14, 2021

No description provided.

@sept-en sept-en added the C-enhancement Category: New feature or request label May 14, 2021
@sept-en sept-en changed the base branch from master to eth-connector May 14, 2021 12:58
Copy link
Member

@birchmd birchmd left a comment

Choose a reason for hiding this comment

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

This is looking pretty good to me so far!

src/admin_controlled.rs Show resolved Hide resolved
src/connector.rs Outdated Show resolved Hide resolved
@sept-en sept-en marked this pull request as ready for review May 14, 2021 15:03
@sept-en sept-en requested a review from artob as a code owner May 14, 2021 15:03
@sept-en sept-en requested review from mrLSD and joshuajbouw May 14, 2021 15:05
@sept-en
Copy link
Contributor Author

sept-en commented May 14, 2021

Btw, the CI is failing because of the internal Github Actions issue reported there.

src/connector.rs Outdated Show resolved Hide resolved
let promises = res.promise_results();
let p = promises[1].clone();
match p.unwrap().status() {
ExecutionStatus::Failure(_) => {}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's worth trying to check specifically the error is raised because of the pause (e.g. by checking the error message), as opposed to some other reason. Just for the sake of completeness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense for me! It's worth adding the same for already existing tests there

tests/test_connector.rs Outdated Show resolved Hide resolved
tests/test_connector.rs Show resolved Hide resolved
Copy link
Member

@mrLSD mrLSD left a comment

Choose a reason for hiding this comment

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

We should change PausedMask logic to enum.

src/lib.rs Outdated Show resolved Hide resolved
src/admin_controlled.rs Show resolved Hide resolved
src/connector.rs Show resolved Hide resolved
Copy link
Member

@mrLSD mrLSD left a comment

Choose a reason for hiding this comment

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

We discussed with @sept-en directly, so for now, the current implementation is pretty enough.

src/admin_controlled.rs Show resolved Hide resolved
src/admin_controlled.rs Show resolved Hide resolved
src/connector.rs Show resolved Hide resolved
src/admin_controlled.rs Outdated Show resolved Hide resolved
src/connector.rs Outdated Show resolved Hide resolved
src/connector.rs Outdated Show resolved Hide resolved
src/connector.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
tests/test_connector.rs Show resolved Hide resolved
@sept-en
Copy link
Contributor Author

sept-en commented May 20, 2021

As we internally discussed with @joshuajbouw @birchmd, we won't use bitflags crate at the current implementation (at least now). The initial idea was to improve the readability and improve the development experience, but we discovered that using this approach will require having additional boilerplate code for Borsh serialization/deserialization, make the AdminControlled trait very specific and not reusable in other possible components/repos. We also agreed that each dependency increases the risk from a security perspective, and it's better not to add additional dependencies if it's not crucial or doesn't give us significant improvements.

Copy link
Member

@birchmd birchmd left a comment

Choose a reason for hiding this comment

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

LGTM nice work!

src/connector.rs Outdated Show resolved Hide resolved
tests/test_connector.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@sept-en sept-en merged commit 92f877d into eth-connector May 27, 2021
@sept-en sept-en deleted the admin-controlled-and-pausable branch May 27, 2021 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants