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

Remove Multisig example contract #1075

Closed
wants to merge 1 commit into from
Closed

Conversation

HCastano
Copy link
Contributor

@HCastano HCastano commented Dec 8, 2021

This PR removes the Multisig example contract in preparation of the (temporary) removal
of the ink_storage::collections crate.

This example is the only one we have which makes use of the ink_storage::{Vec, Stash}
data structures, which at the moment don't play nice with the non-caching data structures
which we want smart contract developers to use (see #1070 for a little bit of context).

This is a good example though, so once we re-work the collections we should bring it
back.

@xgreenx
Copy link
Collaborator

xgreenx commented Dec 8, 2021

You don't need to remove the whole example. You easy can use ink_storage::lazy::Lazy<ink_prelude::vec::Vec<AccountId>> instead of StorageVec and later update it to use a new vector without caching. That example can be useful for people without a new vector=)

@athei
Copy link
Contributor

athei commented Dec 9, 2021

But I think we don't want people to use that either? Doesn't that explode the file sizes?

@xgreenx
Copy link
Collaborator

xgreenx commented Dec 9, 2021

ink_prelude::vec::Vec takes much less space than ink_storage::collections::Vec. The main idea of multisig example is to show how to do a cross-contract call, how to define your own structures, how to manage transactions. It is not production example of the multisig, but it shows well how to work with ink!.

@HCastano
Copy link
Contributor Author

HCastano commented Dec 9, 2021

@xgreenx so the other problem here is the use of Stash, which uses a LazyIndexMap internally. I think it's possible to re-write the contract without using it, but I want to push that down the road a little since we're trying to push for a stable release soon

@xgreenx
Copy link
Collaborator

xgreenx commented Dec 9, 2021

@xgreenx so the other problem here is the use of Stash, which uses a LazyIndexMap internally. I think it's possible to re-write the contract without using it, but I want to push that down the road a little since we're trying to push for a stable release soon

I see. Yes, I think it should use Mapping and use a hash of the transactions as TransactionId instead of the index in Stash=) But if you want to re-write it later, okay. When do you plan to do a stable release? Because it still contains several critical bugs for users=)

@HCastano
Copy link
Contributor Author

HCastano commented Dec 9, 2021

When do you plan to do a stable release? Because it still contains several critical bugs for users=)

@xgreenx So the main things as far as ink! is concerned are

  • Non-payable constructors, since this is a breaking change
    • This is mostly just waiting for UI support
  • Removal of non-caching collections
    • It's easier to add back code than to remove it later
  • Removal of the old off-chain environment
  • Storage deposit support
    • There's a PR open for this, but is waiting on some off-chain env stuff

We think that any other issues can be addressed in minor releases later. Out of curiosity, what issues do you deem as critical? I suspect #982, but anything else?

@athei
Copy link
Contributor

athei commented Dec 10, 2021

Ported the multisig example. However, the off chain tests are failing with an error I don't comprehend. Maybe someone can assist me with debugging: #1078

@xgreenx
Copy link
Collaborator

xgreenx commented Dec 10, 2021

We think that any other issues can be addressed in minor releases later. Out of curiosity, what issues do you deem as critical? I suspect #982, but anything else?

Yes, #982 is critical because it doesn't allow to use of traits fully and no workaround exists.

#1000 has a workaround, but it is related to #809 and I think that resolving it can change the API, so why it is also is critical.

#900 requires additional follow-up change which affects selectors in trait.

Did you clean up all stuff related to that? #906

@HCastano
Copy link
Contributor Author

Yes, #982 is critical because it doesn't allow to use of traits fully and no workaround exists.

On the surface this seems like something that we can add after the 3.0 release without breaking anything for users, but I don't have a lot of insight into this tbh.

#1000 has a workaround, but it is related to #809 and I think that resolving it can change the API, so why it is also is critical.

Again, on the surface this looks like the API could be backwards compatible, but don't quote me on this.

#900 requires additional follow-up change which affects selectors in trait.

I'm gonna defer to @cmichi here since he's been chatting with you in that issue

Did you clean up all stuff related to that? #906

So a bunch of stuff was removed in #1036, but I think there might still be some more to clean up.

Btw, we probably shouldn't be having this discussion here. @cmichi can you enable "Discussions" for the repo so we can move this conversation there?

@HCastano
Copy link
Contributor Author

Closing in favour of #1078.

@HCastano HCastano closed this Dec 10, 2021
@HCastano HCastano deleted the hc-remove-multisig-example branch December 10, 2021 23:36
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