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

wallet: do not store deployed contract script inside Contract account field #3470

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

AliceInHunterland
Copy link
Contributor

Close #3348

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.

The logic of this PR is correct. TestSignMultisigTx fails because contract's witness (an empty one, actually) is not added to the parameter context and parameter context is not aware of contract witnesses while building the resulting tx witnesses:

// GetCompleteTransaction clears transaction witnesses (if any) and refills them with
// signatures from the parameter context.
func (c *ParameterContext) GetCompleteTransaction() (*transaction.Transaction, error) {
tx, ok := c.Verifiable.(*transaction.Transaction)
if !ok {
return nil, errors.New("verifiable item is not a transaction")
}
if len(tx.Scripts) > 0 {
tx.Scripts = tx.Scripts[:0]
}
for i := range tx.Signers {
w, err := c.GetWitness(tx.Signers[i].Account)
if err != nil {
return nil, fmt.Errorf("can't create witness for signer #%d: %w", i, err)
}
tx.Scripts = append(tx.Scripts, *w)
}
return tx, nil
}

@roman-khimov, currently we can't use neo-go wallet sign with contract accounts. We have #2670 for those contracts that have verify method with arguments. Do you think we need to support at least an empty case of verify method right now in neo-go wallet sign? We also need parameter context to still be compatible with the C# behaviour, thus need to check how C# node handles contract witness case in parameter context.

@AnnaShaleva AnnaShaleva changed the title wallet: remove contract script wallet: remove contract script from Contract field Jun 3, 2024
@AnnaShaleva
Copy link
Member

contract's witness (an empty one, actually) is not added to the parameter context and parameter context is not aware of contract witnesses while building the resulting tx witnesses

That's not correct, everything is OK with this code, it can properly handle contract witness.

@AliceInHunterland, you need to explicitly set account's script to null when importing contract account, otherwise an old value of some real signature account's script will be used:

anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ git diff
diff --git a/cli/wallet/wallet.go b/cli/wallet/wallet.go
index 12691683..116d5280 100644
--- a/cli/wallet/wallet.go
+++ b/cli/wallet/wallet.go
@@ -651,6 +651,7 @@ func importDeployed(ctx *cli.Context) error {
                return cli.NewExitError("contract has no `verify` method with boolean return", 1)
        }
        acc.Address = address.Uint160ToString(cs.Hash)
+       acc.Contract.Script = nil
        acc.Contract.Parameters = acc.Contract.Parameters[:0]
        for _, p := range md.Parameters {
                acc.Contract.Parameters = append(acc.Contract.Parameters, wallet.ContractParam{

@AnnaShaleva AnnaShaleva changed the title wallet: remove contract script from Contract field wallet: do not store deployed contract script inside Contract account field Jun 5, 2024
@AnnaShaleva
Copy link
Member

@AliceInHunterland, add this line, add some comment about it and adjust the commit message.

@AliceInHunterland AliceInHunterland marked this pull request as ready for review June 5, 2024 11:10
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.16%. Comparing base (3c471f0) to head (bb2d060).

Current head bb2d060 differs from pull request most recent head b4fdf8c

Please upload reports for the commit b4fdf8c to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3470      +/-   ##
==========================================
+ Coverage   86.12%   86.16%   +0.04%     
==========================================
  Files         331      331              
  Lines       38476    38476              
==========================================
+ Hits        33137    33153      +16     
+ Misses       3809     3798      -11     
+ Partials     1530     1525       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cli/wallet/wallet.go Outdated Show resolved Hide resolved
`Contract` account field should not contain deployed contract script.

Close #3348

Signed-off-by: Ekaterina Pavlova <[email protected]>
@AnnaShaleva AnnaShaleva merged commit 7a7a5d0 into master Jun 5, 2024
18 of 19 checks passed
@AnnaShaleva AnnaShaleva deleted the contract-acc branch June 5, 2024 12:35
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.

Do not put contract script into wallet.Account script for deployed contracts
2 participants