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

Another implementation of multisig #1079

Closed

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Dec 11, 2021

This implementation allows avoiding the storing of the order of transactions. TransactionId is a hash of the encoded transaction.

Each transaction has a status. PartialConfirmed stores the list of confirmations and id of the owners set when that status was created. If we updated the list of owners, that id will mismatch and it means that we need to validate that each confirmation.

I think that this example is simpler in understanding.

It is only an idea. But if you think that it is not bad, I will finish it=)

@athei
Copy link
Contributor

athei commented Dec 11, 2021

If you feel the need to move stuff around at least put it into a seperate commit. The way it is done here bloats the diff and makes it harder to review than necessary. Apart from that I think the idea is good but would require changes to ink-waterfall because the interface changes.

I will give a more in depth review once the diff is cleaned up.

@xgreenx xgreenx changed the base branch from at-port-multisig to master December 11, 2021 23:19
@xgreenx
Copy link
Collaborator Author

xgreenx commented Dec 12, 2021

Rebased to master=)

Copy link
Contributor

@athei athei left a comment

Choose a reason for hiding this comment

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

IMHO this changes too much at the same time without any immediate need. Please let us first get my minimal port PR merged once the waterfall is running again and then we can look into these improvements.

@HCastano HCastano changed the title [Draft] Another implementation of multisig Another implementation of multisig Dec 14, 2021
@HCastano
Copy link
Contributor

HCastano commented Feb 4, 2022

@xgreenx do you want to pick this up again, or should I close it?

@xgreenx
Copy link
Collaborator Author

xgreenx commented Feb 4, 2022

The idea of that change is to avoid iterations throw confirmations and don't store a complex structure in the storage.
It changes the internal implementation. If you think that it is not a bad idea, we can continue either you can close. We can replace the current implementation of multisig or we can mark it as "multisig_v2"

@HCastano
Copy link
Contributor

HCastano commented Feb 4, 2022

If you don't mind I'd move forward with it and see how the ink-waterfall benchmarks look.

I also wouldn't keep two Multisig examples around, so "improving" the current example is fine with me

@HCastano
Copy link
Contributor

This is pretty stale so I'm gonna close this. If you ever want to pick this back up feel free to do so

@HCastano HCastano closed this May 11, 2022
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