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

Allow complex contract signature schemes in wallet.Account #3256

Merged
merged 2 commits into from
Dec 21, 2023

Conversation

fyfyrchik
Copy link
Contributor

Close #3015

Implement second option from #3015 (comment)
In the first one (interface instead of wallet.Account) we still have a problem with network fee calculation: actor modifiers are executed after network fee is calculated and there are no hooks to change invocation script.

This implementation disallows providing signature as an argument (though in a backward compatible way).
It is a restriction, but not a sad one: if we have a private key, we can always add a simple signer and provide its address to verify(), which can use runtime.CheckWitness to effectively perform the same check (with proper scopes it could be exactly the same).

Also, not sure about the meaning of CanSign, so didn't touch it here.

TBH I would remove func argument in the NewContractSigner in neotest, it introduces a subtle dependency on the condition order in SignTx. Other option is to provide an integer but it is also a bit annoying. As an example, there is wallet.NewContractAccount which can be used instead of notary.FakeContractAccount in complex cases and which could be reused in tests.

We have 2 similar usecases for complex Verify scenarios, and in every in test we use constant arguments, could be more concise:

s := neotest.NewContractSigner(e.Hash, func(*transaction.Transaction) []any { return []any{acc.ScriptHash()} })
// could be
s := neotest.NewContractSigner(e.Hash, acc.ScriptHash())

Initially I wanted to add type-safe VerifyAccount constructors to rpcbinding but is doesn't provide much benefit compared to effort taken.

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

A really nice one, and I like the contract signer simplification. With this PR it would be easier to implement #2670, so maybe we can include #2670 to our nearest plans, @roman-khimov, what do you think?

pkg/wallet/account.go Outdated Show resolved Hide resolved
@roman-khimov
Copy link
Member

#2670 is good, but we still have some better ones in the queue. Let's discuss this after 0.105.0 release.

…3015

This was recently added in neotest, but working with the real RPC is
still not enjoyable. This commit extends `wallet.Account` with
invocation script builder to aid network fee calculations and signing.

Signed-off-by: Evgenii Stratonikov <fyfyrchik@runbox.com>
Refs nspcc-dev#3245
Refs nspcc-dev#3233

Signed-off-by: Evgenii Stratonikov <fyfyrchik@runbox.com>
@roman-khimov roman-khimov merged commit 56d32a0 into nspcc-dev:master Dec 21, 2023
@fyfyrchik fyfyrchik deleted the fix-signer branch December 22, 2023 09:11
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.

Decouple actor from implementation of transaction signer
3 participants