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

TESTCOV: Sputnik Factory: Ownership #146

Merged

Conversation

Sleep-No-More
Copy link
Contributor

Draft PR for Sputnik Factory Ownership test coverage #121

Tests will be written in Rust in a test module inside a respected file (lib.rs).

Recommendations are very welcome.

@Sleep-No-More Sleep-No-More added P-High Priority: High C-Testing Unit testing / integration testing labels Mar 23, 2022
@Sleep-No-More Sleep-No-More linked an issue Mar 23, 2022 that may be closed by this pull request
4 tasks
@Sleep-No-More
Copy link
Contributor Author

We are not compleatly sure that solution for [owner_can_be_a_dao_account] is correct.

@Sleep-No-More Sleep-No-More marked this pull request as ready for review March 29, 2022 13:13
Copy link
Contributor

@TrevorJTClarke TrevorJTClarke left a comment

Choose a reason for hiding this comment

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

See comments below, thanks!

sputnikdao-factory2/src/lib.rs Outdated Show resolved Hide resolved
sputnikdao-factory2/src/lib.rs Outdated Show resolved Hide resolved
sputnikdao-factory2/src/lib.rs Outdated Show resolved Hide resolved
sputnikdao-factory2/src/lib.rs Outdated Show resolved Hide resolved
@Sleep-No-More
Copy link
Contributor Author

Sleep-No-More commented Mar 29, 2022

Changed as required @TrevorJTClarke thanks a lot for the clarification.

Copy link
Contributor

@TrevorJTClarke TrevorJTClarke left a comment

Choose a reason for hiding this comment

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

Looks like almost complete! Just need to change 1 test - see comment

Comment on lines 477 to 482

factory.create(bob(), "{}".as_bytes().to_vec().into());

factory.set_owner(alice());

assert_eq!(factory.get_owner(), alice())
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is wrong - it needs to use the DAO account in the set_owner method, then check that the DAO account is owner in get_owner.
Since you're using bob as the name, it will be something like bob.sputnik-dao.near as the DAO account.
So make sure to change set_owner to be the actual full dao name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the changes, logic should be correct now.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, all fixed - thanks!

@TrevorJTClarke TrevorJTClarke merged commit f7bc52c into near-daos:main Apr 5, 2022
@Sleep-No-More Sleep-No-More deleted the TESTCOV-Sputnik-Factory-Ownership branch April 5, 2022 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Testing Unit testing / integration testing P-High Priority: High
Projects
Status: Ready For QA
Development

Successfully merging this pull request may close these issues.

TESTCOV: Sputnik Factory: Ownership
4 participants