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.

Sign up for GitHub

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

Jump to bottom

Collections: changes after merge #8

Merged
merged 25 commits into from
Aug 19, 2024
Merged

Collections: changes after merge #8

merged 25 commits into from
Aug 19, 2024

Conversation

gangov
Copy link
Collaborator

@gangov gangov commented Aug 14, 2024

No description provided.

@gangov gangov self-assigned this Aug 14, 2024
@gangov gangov requested a review from ueco-jb August 14, 2024 15:37
@gangov gangov requested a review from ueco-jb August 14, 2024 16:06
@gangov gangov requested a review from ueco-jb August 15, 2024 07:33
gangov added 15 commits August 15, 2024 12:37
@gangov
failing path for batch_balance_of
7f1e3ab
@gangov
failing test case for when the user tries to authorize himself
ff8dc8c
@gangov
adds tests for when the user doesn't have enough to spend
29320e1
@gangov
adds a test for safe_transfer
8d73640
@gangov
removes a test and refactors the logic on how we handle transfer
cd380b2
@gangov
safe batch transfer test
80d7efa
@gangov
more tests for the failing path when dealing with safe_batch_transfer
dd84a14
@gangov
adds a failing test case for when the user is not authorized to mint
9ffe070
@gangov
more tests for failing test case when batch_minting
250dbf5
@gangov
test for when burn fails
263c4fe
@gangov
more tests for when batch_burn fails
7f0aedd
@gangov
test for when set_uri fails
15e6afc
@gangov
test for set collection uri
ada922a
@gangov
test case for when looking for a uri when such is not set
bc06a71
@gangov
adds a few tarpaulin macros to ignore test cases which we cannot cover
8b6f972
ueco-jb
ueco-jb reviewed Aug 15, 2024
View reviewed changes
Copy link
Member

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

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

Please also add events during the messages:


        env.events()
            .publish(("initialize", "XYK LP token_a"), token_a);
        env.events()
            .publish(("initialize", "XYK LP token_b"), token_b);

Like those.

@gangov
Copy link
Collaborator Author

gangov commented Aug 16, 2024

just as a heads up - the lint fails locally because of the #[cfg(not(tarpaulin_include))] macro, here it seems to be okay

@gangov gangov requested a review from ueco-jb August 16, 2024 08:01
ueco-jb
ueco-jb approved these changes Aug 19, 2024
View reviewed changes
Copy link
Member

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

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

LGTM
Good for merge after fixing my comments
You can address the wrong auth assertion from my comment here:
#7 (review)

@gangov gangov merged commit ce19935 into master Aug 19, 2024
3 checks passed
@ueco-jb ueco-jb deleted the feature/changes-to-collections branch August 19, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ueco-jb ueco-jb ueco-jb approved these changes

Assignees

@gangov gangov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gangov @ueco-jb