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.

Sign up for GitHub

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

Jump to bottom

[Fix]: try to fix account.Lock check #3791

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[Fix]: try to fix account.Lock check #3791

wants to merge 2 commits into from

Conversation

nan01ab
Copy link
Contributor

@nan01ab nan01ab commented Feb 26, 2025 •
edited
Loading

Description

Try to fix account.Lock check issue.

Fixing this issue may require more discussion.
Especially in case of multi-sig and changes in api behavior.

Fixes #3789

Type of change

  • Optimization (the change is only an optimization)
  • Style (the change is only a code style for better maintenance or standard purpose)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

cschuchardt88
cschuchardt88 reviewed Feb 26, 2025
View reviewed changes
src/Neo/Wallets/Wallet.cs
// Try to sign self-contained multiSig

Contract multiSigContract = account.Contract;
if (account.Lock) continue;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe an exception is best here AccessDeniedException. This would fix problems with users not knowing the reason why Sign is returning false. Please update the description as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe an exception is best here AccessDeniedException. This would fix problems with users not knowing the reason why Sign is returning false. Please update the description as well.

This will lead to a lot of modifications.

Copy link
Member

Choose a reason for hiding this comment

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

I have no good answer for you 😞

@cschuchardt88
Copy link
Member

cschuchardt88 commented Feb 26, 2025 •
edited
Loading

Should this apply to contract based accounts? These are on chain contracts, that have their own verify method. So i think it's not needed for them.

How you check is if the script and parameters list is null or empty.

@nan01ab
Copy link
Contributor Author

nan01ab commented Feb 26, 2025

Should this apply to contract based accounts? These are on chain contracts, that have their own verify method. So i think it's not needed for them.

How you check is if the script and parameters list is null or empty.

I don't know why check the script and parameters list is null or empty. :(.
neo-go doesn't have check for this.

@nan01ab
Copy link
Contributor Author

nan01ab commented Feb 26, 2025

no multi-sig is used now, and neo-go doesn't have this feature too.

@cschuchardt88
Copy link
Member

cschuchardt88 commented Feb 26, 2025 •
edited
Loading

no multi-sig is used now, and neo-go doesn't have this feature too.

  1. We don't follow neoGo, they follow NEP.
  2. This is core, we follow NEPs too.
  3. Your mistaken about that

https://github.com/nspcc-dev/neo-go/blob/0d8c751e50951ad84cec8b89cf6ea1a9e0f4f878/pkg/wallet/account.go#L271-L273

@nan01ab
Copy link
Contributor Author

nan01ab commented Feb 26, 2025

no multi-sig is used now, and neo-go doesn't have this feature too.

  1. We don't follow neoGo, they follow NEP.
  2. This is core, we follow NEPs too.
  3. Your mistaken about that

https://github.com/nspcc-dev/neo-go/blob/0d8c751e50951ad84cec8b89cf6ea1a9e0f4f878/pkg/wallet/account.go#L271-L273

Not used in signing consensus messages

@nan01ab
Copy link
Contributor Author

nan01ab commented Feb 26, 2025 •
edited
Loading

no multi-sig is used now, and neo-go doesn't have this feature too.

  1. We don't follow neoGo, they follow NEP.
  2. This is core, we follow NEPs too.
  3. Your mistaken about that

https://github.com/nspcc-dev/neo-go/blob/0d8c751e50951ad84cec8b89cf6ea1a9e0f4f878/pkg/wallet/account.go#L271-L273

I mean if it's not used now, the change of this API has less impact on current system behaviour.

@cschuchardt88
Copy link
Member

cschuchardt88 commented Feb 26, 2025 •
edited
Loading

Let just not encrypt wallet password too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cschuchardt88 cschuchardt88 cschuchardt88 left review comments

@shargon shargon shargon approved these changes

@Jim8y Jim8y Jim8y approved these changes

Assignees
No one assigned
Labels
Core Ready to Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No check whether the account is locked when signing consensus messages
4 participants
@nan01ab @cschuchardt88 @shargon @Jim8y