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

System.checkAuthority using getContractMetadata #74

Merged
merged 13 commits into from
Jul 12, 2024

Conversation

joticajulian
Copy link
Contributor

@joticajulian joticajulian commented Feb 15, 2024

Brief description

This PR takes advantage of the new system contract called "get_contract_metadata", which is used to determine if an account has a contract or not. This is useful to improve the security of the assets (see references).

Changes:

  • New function System.getContractMetadata
  • The previous System.checkAuthority has been renamed to System.checkAuthorityLegacy
  • New function System.checkAuthority which calls getContractMetadata internally.
  • New function System.checkCallContractAuthority which calls the checkAuthority and fill the arguments.
  • System.requireAuthority updated.

References

Checklist

  • I have built this pull request locally
  • I have ran the unit tests locally
  • I have manually tested this pull request
  • I have reviewed my pull request
  • I have added any relevant tests

Demonstration

I deployed 3 contracts in mainnet: smart wallet, assets contract, and third party contract. These contracts interact between them to demonstrate the advantages of the new system call. They are working as expected.
You can follow the tests described in this post but using these addresses for mainnet:

Copy link
Member

@mvandeberg mvandeberg left a comment

Choose a reason for hiding this comment

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

With the implicit calls to getArguments and getCaller in function signatures, do we want to add some internal caching so when those calls are made multiple times, we can short circuit and return the cached object?

@mvandeberg
Copy link
Member

I have also requested @sgerbino's review on this PR to ensure correctness and usability before we release it to contract devs.

@joticajulian
Copy link
Contributor Author

With the implicit calls to getArguments and getCaller in function signatures, do we want to add some internal caching so when those calls are made multiple times, we can short circuit and return the cached object?

Good suggestion. I just added a cache value for these 2 functions.

@mvandeberg
Copy link
Member

Sorry for the delay in review. LGTM, I want @sgerbino to look at it as well before we merge.

Copy link
Member

@mvandeberg mvandeberg left a comment

Choose a reason for hiding this comment

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

Looking at this again, there are no tests for this new behavior. We probably need to add contract metadata to MockVM so we can be sure this authority check works in the scenario it is intended to address.

@joticajulian
Copy link
Contributor Author

I've been very busy in other topics. I'd appreciate if someone can work in the unit tests.

@mvandeberg mvandeberg changed the base branch from master to check-authority July 12, 2024 20:32
@mvandeberg mvandeberg merged commit 76c12e2 into koinos:check-authority Jul 12, 2024
@mvandeberg mvandeberg mentioned this pull request Jul 15, 2024
1 task
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.

3 participants