contract: make multichain contract versioned #558
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ppca
changed the title
Terraform Feature Environment (dev-558)Terraform Initialization ⚙️
|
ppca
force-pushed
the
xiangyi/versioned_contract
branch
from
9a7095c to
1e3a7e8
Compare
ChaoticTempest
previously approved these changes
Apr 12, 2024
ppca
force-pushed
the
xiangyi/versioned_contract
branch
from
776d4e8 to
a7dd06a
Compare
Closed
volovyks
reviewed
Apr 16, 2024
| self.request_counter = counter; | ||
| } | ||
|
|
||
| /// This is the root public key combined from all the public keys of the participants. |
There was a problem hiding this comment.
We will need to refactor the contract. It is getting too big. But for now, let's keep public user-facing functions on top.
There was a problem hiding this comment.
Should we separate impl into 2 or 3 blocks? Like: user-facing functions, interactions with node and init, helper functions. cc @ChaoticTempest
There was a problem hiding this comment.
Yeah, that would be ideal to keep public facing functions at the top of a lib.rs with a private.rs to do the helper functions. But most of this is just some house keeping. For the purpose of having things visible as more people come to read the contract, the fastest thing to do right now is just surface the most important public functions to the very top without much refactoring into separate files. We can do this in a separate PR
| #[test] | ||
| fn test_old_state_can_be_migrated_to_v0() -> anyhow::Result<()> { | ||
| let old_contract = MpcContract::init(3, BTreeMap::new()); | ||
| env::state_write(&old_contract); |
There was a problem hiding this comment.
This code is not executed in Wasmer. Where state is written to?
We need to write an integration test using workspaces to test this logic. And to test it properly we will need to have actual old and new states.
There was a problem hiding this comment.
This is writing to the current env's state variable. the migration function reads from the contract env's state variable to get the old state's fields, then return the new contract. So this test mimics the state variable.
The hard part about using workspaces and having actual old state, is we would need to maintain the old contract with #[nearbingen] and compile it, and also the new contract with #[nearbingen] and compile it, this is impossible to achieve with current code.
Let me know if you can find a way to do that.
There was a problem hiding this comment.
We can have a dummy structure representing an old state V0, then V1 will be the current one. I don't think that is necessary now. We will add such a test on the next state change.
There was a problem hiding this comment.
The env::state_write writes to a MockBlockchain in the case of a unit test. We don't need to go into workspaces for this since all this is testing is the serialization and that's what we want to test directly.
We can have a dummy structure representing an old state V0, then V1 will be the current one. I don't think that is necessary now. We will add such a test on the next state change.
yeah, we need to maintain a list of all structs that we used before in these tests
| } | ||
|
|
||
| /// This is the root public key combined from all the public keys of the participants. | ||
| pub fn public_key(self) -> PublicKey { |
There was a problem hiding this comment.
We should proxy all user-facing functions. All the tmp and service functions are not that important.
There was a problem hiding this comment.
what do you mean by proxy all user-facing functions?
There was a problem hiding this comment.
Yes, wrapping will not help with breaking changes since we can do conversions inside of these functions.
Let's refactor the contract and have a clear separation between contract API and service functions. For example, since it's hard to read now.
ChaoticTempest
previously approved these changes
May 10, 2024
ppca
force-pushed
the
xiangyi/versioned_contract
branch
from
7406421 to
795cfde
Compare
ChaoticTempest
approved these changes
May 13, 2024
Terraform Feature Environment Destroy (dev-558)Terraform Initialization ⚙️
|
mikedotexe
reviewed
May 17, 2024
| #[derive(BorshDeserialize, BorshSerialize, PanicOnDefault)] | ||
| #[derive(BorshDeserialize, BorshSerialize, Debug)] | ||
| pub enum VersionedMpcContract { | ||
| V0(MpcContract), |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will be our contract
I put majority of the fn logics in VersionedMpcContract implementation, and added functions that read data from contract, e.g.
So each version could have different format of storing data and we could add code to above fn to read data from each version.