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

A0-3181: Bump toolchain version #21

Merged
merged 10 commits into from
Sep 4, 2023
Merged

A0-3181: Bump toolchain version #21

merged 10 commits into from
Sep 4, 2023

Conversation

deuszx
Copy link
Collaborator

@deuszx deuszx commented Sep 1, 2023

drink is not compatible with Rust version used in this project, it requires 1.72+.

After bumping to matching version of nightly toolchain, I was getting

Specialization impl does not specialize any associated items

error for all occurrences of:

impl X for Y {}

(i.e. empty impl of a trait that has all of its methods implemented as default fn).

I can't remove that b/c then there won't be any methods (no method marked with #[ink(message)] macro) available on the contract.

To fix the above I moved all of the default impls to respective contract files.

For OpenBrush-only traits (like PSP22 and PSP22Metadata) I simply copied their default implementations from the OB repository.

This also is an intermediate step towards an OB-less future.

NOTE that this doesn't compile commit-by-commit. I decided to split the changes into commits that show why I had to do things I did, rather than self-contained changes.

}

#[ink(message)]
fn transfer_from(

Choose a reason for hiding this comment

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

this code is duplicated in psp22/lib.rs. Can't you refactor it to some native Rust trait and use a blanket impl?

}
y
}

impl PSP22 for PairContract {
#[ink(message)]
fn transfer_from(

Choose a reason for hiding this comment

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

again, a lot of this code is repeated between other contracts and could be reused

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a lot, just one method. Here we are actually still using default fns from OpenBrush's PSP22 implementation and overwriting just transfer_from. There is a separate epic to get rid of OB entirely and @h4nsu is working on it.

@deuszx
Copy link
Collaborator Author

deuszx commented Sep 4, 2023

Since this is a "temporary state" (until we remove all OB dependencies from the repo entirely) I think it's OK to merge as-is. We can revive these discussions when we're "more ready" for them.

@deuszx deuszx merged commit 41935cb into main Sep 4, 2023
2 checks passed
@deuszx deuszx deleted the bump-rust branch September 4, 2023 15:41
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.

3 participants