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

Multisig example #1047

Closed
wants to merge 26 commits into from
Closed

Multisig example #1047

wants to merge 26 commits into from

Conversation

iFrostizz
Copy link
Contributor

@iFrostizz iFrostizz commented Jun 12, 2024

Write a multisig example where someone can propose an arbitrary external call. People can then vote on this proposition. Once the quorum is reached, anyone can execute this external call.

@iFrostizz
Copy link
Contributor Author

Blocked by raw bytes return from call_function

@iFrostizz iFrostizz force-pushed the multisig_example branch 2 times, most recently from 1eaa311 to aa51f3e Compare July 8, 2024 12:26
@iFrostizz iFrostizz marked this pull request as ready for review July 8, 2024 13:38
Copy link
Contributor

@richardpringle richardpringle left a comment

Choose a reason for hiding this comment

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

A "multisig" shouldn't just allow anyone and everyone to vote. The voters should be specified in advance

x/programs/cmd/simulator/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/wasmlanche-sdk/src/types.rs Outdated Show resolved Hide resolved
x/programs/rust/wasmlanche-sdk/src/program.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/multisig/Cargo.toml Outdated Show resolved Hide resolved
x/programs/rust/examples/multisig/Cargo.toml Outdated Show resolved Hide resolved
x/programs/rust/examples/multisig/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/multisig/src/lib.rs Outdated Show resolved Hide resolved
to,
method,
data,
value,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the amount of native tokens sent along with the execution.
I wanted to keep it simple, having some kind of value contribution will make it more complex since we need to handle the value amount (with a floor to ensure the account always has enough funds), and refunds if the execution fails.

Copy link
Contributor Author

@iFrostizz iFrostizz Jul 10, 2024

Choose a reason for hiding this comment

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

So the value is just passed by the actor who executes the proposal. It's probably a bit unfair, but it was a bit better than just passing 0.

x/programs/rust/examples/multisig/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/multisig/src/lib.rs Outdated Show resolved Hide resolved
@iFrostizz iFrostizz mentioned this pull request Jul 10, 2024
@iFrostizz
Copy link
Contributor Author

Currently blocked by address passing #1121

@iFrostizz iFrostizz requested a review from samliok as a code owner July 11, 2024 14:21
x/programs/rust/examples/multisig/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/multisig/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/multisig/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/multisig/src/lib.rs Show resolved Hide resolved
x/programs/rust/examples/multisig/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/multisig/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/multisig/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/multisig/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/multisig/src/lib.rs Outdated Show resolved Hide resolved
x/programs/rust/examples/multisig/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@samliok samliok left a comment

Choose a reason for hiding this comment

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

overall this program is a really cool example 🚀!

One note: important to have thorough commenting on examples, since this is prob the first place devs will look. I would add helpful comments to things like public functions, statekeys, struct fields, etc...


#[state_keys]
pub enum StateKeys {
LastProposalId,
Copy link
Contributor

Choose a reason for hiding this comment

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

definitely not in this PR, but it seems redundant to store the LastProposalId when it's effectively the length of Proposals(u64). Maybe we should expose a host function that gets the length/size of a statekey?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(this key was renamed to NextProposalId instead)
I'm not sure if we could have such a host function, the data may not be stored in a contiguous space of memory and it may be hard to have such an information. What we may want though is a Vec library program (if we are able to write stateful libraries in the future) that stores this information. This is a good remark !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason we might want this Vec library is that storing a Vec as a state key is going to make it costly to pull it from storage because we can only get the full vec.

Copy link
Contributor

@samliok samliok Jul 19, 2024

Choose a reason for hiding this comment

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

yea, a wayyyys back the initial thought was to have host functions that would reduce the amount of data pulled into WASM(stuff like getting values in a map, vec, etc...). we don't have host functions for maps anymore because of StateKeys but vecs are a little different.

under the hood a Vecs could be stored differently, and come with additional get functions like get_index. Could also do something similar to solidity, and just handle them i differently under the hood in get/set methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's very interesting! I don't know if we really want this at the vm level though, it feels more like a program-level thing, the vm may not need to be aware of that, but I'm happy to try both !!

x/programs/rust/examples/multisig/src/lib.rs Show resolved Hide resolved
x/programs/rust/examples/multisig/src/lib.rs Outdated Show resolved Hide resolved
@iFrostizz
Copy link
Contributor Author

iFrostizz commented Jul 22, 2024

DeferDeserialize cannot be used in the storage https://github.com/ava-labs/hypersdk/pull/1047/files#diff-a262134f908ac01d356baed6d18b02ea08af43560006cd3255dff48394e6cdebR12 because the deserialization implementation by nature exhausts all bytes

reader.read_to_end(&mut inner)?;
meaning that when it is deserialized (after calling get let's say), then it's going to read all bytes expected from the DeferDeserialize and will read all bytes of the subsequent fields too. Solutions:

  1. Add a AsRef<[u8]> implementation
  2. Write the DeferDeserialize at the end of all fields

It's a security issue if this DeferDeserialize cannot be deserialized in a struct and may create some DOS vectors.

@iFrostizz
Copy link
Contributor Author

superseded by #1581

@iFrostizz iFrostizz closed this Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants