-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[Neo Rpc Methods] fix contact rpc methods parameters #3485
base: master
Are you sure you want to change the base?
Conversation
/// This method is used to test your VM script as if they ran on the blockchain at that point in time. | ||
/// This RPC call does not affect the blockchain in any way. | ||
/// </remarks> | ||
/// <param name="scriptHash">Smart contract scripthash. Use big endian for Hash160, little endian for ByteArray.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use big endian for Hash160
It's LE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check it again, cause its from the official document.
/// <param name="useDiagnostic">Optional. Flag to enable diagnostic information.</param> | ||
/// <returns>A JToken containing the result of the invocation.</returns> | ||
[RpcMethodWithParams] | ||
protected internal virtual JToken InvokeFunction(string scriptHash, string operation, ContractParameter[] args = null, Signer[] signers = null, bool useDiagnostic = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behaviour diverges from an old one. This method should accept the list of signers with witnesses as the fourth parameter, not only the list of signers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It never worked. And not bing of any use at anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And not bing of any use at anywhere.
You don't know that. It's an external API and it's a very widely used one. Your changes can easily break real applications. Refactorings must not break compatibility.
|
||
namespace Neo.Plugins.RpcServer.Model; | ||
|
||
public class SignerOrWitness |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make a base
class for common stuff? Than two different classes one, being Signer
and other being Witness
. This way we can get Signer
attributes like Rules
or other properties that separate the two classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make it work first, optimize it later. I think you are better than me to make the code more elegent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may take a look at https://github.com/nspcc-dev/neo-go/blob/e1d5ac8557d7e087158e6a766d059913a56df79f/pkg/neorpc/types.go#L75 and https://github.com/nspcc-dev/neo-go/blob/e1d5ac8557d7e087158e6a766d059913a56df79f/pkg/neorpc/types.go#L83 as a reference, both Signer and Witness should be accepted at the same time.
@AnnaShaleva please check latest update to make it |
throw new RpcException(CreateInvalidParamError<Signer>(token)); | ||
} | ||
} | ||
throw new RpcException(CreateInvalidParamError<Signer>(token)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would adding an else
statement here make it clearer?
|
||
namespace Neo.Plugins.RpcServer.Model; | ||
|
||
public class SignerWithWitness(Signer? signer, Witness? witness) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to fix the linter comment.
}).ToArray(); | ||
} | ||
|
||
private static UInt160 AddressToScriptHash(string address, byte version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we already had this helper somewhere else. If so, can we reuse the existing helper?
return new[] { signerOrWitness }; | ||
} | ||
} | ||
throw new RpcException(RpcError.InvalidParams.WithData($"Invalid SignerOrWitness format: {token}")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
/// This method is used to test your VM script as if they ran on the blockchain at that point in time. | ||
/// This RPC call does not affect the blockchain in any way. | ||
/// </remarks> | ||
/// <param name="scriptHash">Smart contract scripthash. Use big endian for Hash160, little endian for ByteArray.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not resolved.
@AnnaShaleva updated this pr to mock the complete http request process, now should be able to work without miss any detail. |
Description
This pr updates the rpc plugin contract related methods.
Fixes # (issue)
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: