Add command to download a contract #209
Conversation
…al chain of choice
|
The one thing I dislike about the current implementation is that the way of parameter entry to the download command is inconvenient. Selecting the destination chain is ok, then you can still have the contract hash you want to download on your clipboard for pasting, but the RPC url of the source chain (where we download the contract from) has to be typed manually. You can't seem to switch windows without losing the drop down box. I thought about having the option to store the "source rpc url" in some configuration file such that it can be selected from a list (or if the input field is empty takes the value from the config). What would be the right place? The |
I left a comment about this in the code. TL;DR user should be able to specify 'mainnet' or 'testnet' in addition to a full URL.
Express doesn't know anything about the |
We can ignore this comment. It's actually for the |
Add {} around exceptions
Rename ExpressOracle
|
Note that I did not yet move all logic down to Btw do you prefer 1 commit per issue or are you fine with the 1 commit for all I've been doing so far? |
I don't typically review commit by commit. I like looking at all the changes at once. So whatever works better for you |
src/neoxp/Node/NodeUtility.cs
Outdated
| var stateEquals = localContract.ToJson().ToByteArray(false) | ||
| .SequenceEqual(state.ToJson().ToByteArray(false)); |
There was a problem hiding this comment.
There is probably a better way of doing this, but at least it works (where Equals and == don't).
src/neoxp/Node/NodeUtility.cs
Outdated
| // to avoid having contracts with duplicate id's. This is important because the contract id is part of the | ||
| // StorageContext used with Storage syscalls and else we'll potentially override storage keys or iterate | ||
| // over keys that shouldn't exist for one of the contracts. | ||
| if (state.Id <= LastUsedContractId(snapshot)) |
There was a problem hiding this comment.
Nit: Why not just always use the next available ID? Since we have to handle the case where the local id != remote id, why not simplify the logic?
There was a problem hiding this comment.
I tried to keep it the same as the remote contract whenever possible. Always using the last is also an option.
There was a problem hiding this comment.
I think it's simpler just to issue a new local contract id
| ContractCommand.OverwriteForce.StorageOnly => (false, false), | ||
| _ => throw new NotSupportedException($"Invalid OverwriteForce value {force}"), | ||
| }; | ||
|
|
There was a problem hiding this comment.
Note, I pulled GetSnapshot into PersistContract so that the snapshot lifetime can be managed with a using statement
src/neoxp/Node/NodeUtility.cs
Outdated
| ContractCommand.OverwriteForce.All => (true, true), | ||
| ContractCommand.OverwriteForce.ContractOnly => (true, false), | ||
| ContractCommand.OverwriteForce.None => (false, false), | ||
| ContractCommand.OverwriteForce.StorageOnly => (false, false), |
There was a problem hiding this comment.
| ContractCommand.OverwriteForce.StorageOnly => (false, false), | |
| ContractCommand.OverwriteForce.StorageOnly => (false, true), |
There was a problem hiding this comment.
Ouch, bad copy/paste error
src/neoxp/Node/NodeUtility.cs
Outdated
| if (!localContract.Hash.Equals(state.Hash) | ||
| || localContract.UpdateCounter != state.UpdateCounter | ||
| || !localContract.Nef.ToArray().SequenceEqual(state.Nef.ToArray()) | ||
| || !localContract.Manifest.ToJson().ToByteArray(false).SequenceEqual(state.Manifest.ToJson().ToByteArray(false))) |
There was a problem hiding this comment.
At this point we found a contract locally with the same hash as state.hash (the remote contract) at L313. Therefore
!localContract.Hash.Equals(state.Hash)is always false and seems unnecessary to check again.
With the force argument being StorageOnly || NONE. The only check we should do here is making sure that the contract didn't change which otherwise looks fine.
I thought #209 (comment)) requested a detailed error what is wrong but those changes are now gone and we actually get a generic error instead. Why is that?
|
I tried downloading the |
|
As an end user you have no idea what the error means. I think we should catch it and throw something along the lines of “contract not found on remote server”.
… On 15 Apr 2022, at 18:13, Harry Pierson ***@***.***> wrote:
@devhawk commented on this pull request.
In src/neoxp/Node/NodeUtility.cs:
> + : throw new Exception($"Null \"{nameof(localRootIndex)}\" in state height response");
+ }
+
+ var stateRoot = await stateAPI.GetStateRootAsync(height);
+
+ // rpcClient.GetContractStateAsync returns the current ContractState, but this method needs
+ // the ContractState as it was at stateHeight. ContractManagement stores ContractState by
+ // contractHash with the prefix 8. The following code uses stateAPI.GetStateAsync to retrieve
+ // the value with that key at the height state root and then deserializes it into a ContractState
+ // instance via GetInteroperable.
+
+ var key = new byte[21];
+ key[0] = 8; // ContractManagement.Prefix_Contract
+ _contractHash.ToArray().CopyTo(key, 1);
+
+ var contractStateBuffer = await stateAPI.GetStateAsync(
Good catch. I have a solution for this in the sync version of GetState I wrote for bctk library: https://github.com/ngdenterprise/neo-blockchaintoolkit-library/blob/master/src/bctklib/persistence/RpcClientExtensions.cs#L35
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.
|
|
This looks ready to go. I'll have @JohndeVadoss give it a quick once over before merging |
…from a remote network into our local chain of choice. This is part of an effort to make it easier to make our development (private) chain reflect the final network we want to deploy on. This is part 1, where part 2 will be on the
neo3-visual-trackerrepo. The end result could look like shown in the video.contract-download-generate-2022-03-15_12.27.05.mp4
.