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

APB - 46 Precompiled contract to allow ED25519 signature verification #589

Merged
merged 5 commits into from Sep 4, 2018

Conversation

Projects
None yet
4 participants
@alexandrulaurus
Copy link
Contributor

commented Aug 9, 2018

Description

This adds a new precompiled contract that validates a if a message was signed by a specific account.

     * input
     * |
     * | message hash (32 bytes) | public key (32 bytes) | signature bytes (64 bytes) |
     * <p>
     * where
     * - message hash = keccak 256 hashed message
     * - public key = public key of the ED25519 key pair
     * - signature = 64 bytes of the ED25519 signature

Important: There is NO solidity support for this functionality to be mapped to a function due to the limitations of the aion_fastvm.

Type of change

  • Bug fix.
  • New feature.
  • Enhancement.
  • Unit test.
  • Breaking change (a fix or feature that causes existing functionality to not work as expected).
  • Requires documentation update.

Testing

  • EDVerifyContractTest.java

Verification

  • I have self-reviewed my own code and conformed to the style guidelines of this project.
  • New and existing tests pass locally with my changes.
  • I have added tests for my fix or feature.
  • I have made appropriate changes to the corresponding documentation.
  • My code generates no new warnings.
  • Any dependent changes have been made.
byte[] pubKey = new byte[32];
byte[] sig = new byte[64];

try {

This comment has been minimized.

Copy link
@aionick

aionick Aug 10, 2018

Collaborator

All LGTM. Just wondering, is it worth it to return an error if the input length is larger than expected, since this may indicate that the user has incorrectly understood the format?

This comment has been minimized.

Copy link
@alexandrulaurus

alexandrulaurus Aug 10, 2018

Author Contributor

@aionick What do you think about creating a new ResultCode like ExecutionResult.ResultCode.INVALID_INPUT? Are these codes documented for the clients that use the precompiled contracts? So I know if I need to update the docs.

This comment has been minimized.

Copy link
@aionick

aionick Aug 10, 2018

Collaborator

I'd like to look into this. From my understanding ResultCode is only used internally and never makes it out to the user. I may be wrong here. But I do think we need a little more flexibility in the precompileds for documenting errors. @yao-sun @AionJayT what do you guys think?

This comment has been minimized.

Copy link
@AionJayT

AionJayT Aug 22, 2018

Collaborator

@centrys-david, we can create the case to handle the INVALID_INPUT. But it requires a few places change.
@aionick we definitely need to define how to use return code in case the developer misusing.

This comment has been minimized.

Copy link
@alexandrulaurus

alexandrulaurus Aug 23, 2018

Author Contributor

@AionJayT @aionick

There are 2 cases here:

  1. Smart contract client
    I don't know how to throw/return an error from the precompiled contract that can be interpreted by the aion_fastvm and solidity compiler. The best practices are using require so there needs to be a documentation for the client to say something like:
...
require(hash.length == 32)
require(pub.lenght == 32)
...
edveirfy(hash, pub, ...)
...
  1. Java client
    If someone wants to integrate this precompiled contract on a lower level (eg. calling it directly) there is a minium amount of logging here that one can check on the kernel when developing.

Still, I think documentation is our best choice for now and a new ExecutionResult.ResultCode.INVALID_INPUT

Let me know what you think.

@AionJayT AionJayT self-requested a review Aug 22, 2018

@alexandrulaurus alexandrulaurus force-pushed the Centrys:APB-46-return-address branch from bcac62e to dbdcee0 Aug 23, 2018

@aion82

aion82 approved these changes Aug 24, 2018

@@ -28,6 +28,7 @@
import org.aion.mcf.core.AccountState;
import org.aion.mcf.db.IBlockStoreBase;
import org.aion.precompiled.contracts.ATB.TokenBridgeContract;
import org.aion.precompiled.contracts.EDVerifyContract;

This comment has been minimized.

Copy link
@aion82

aion82 Aug 24, 2018

Ahmmed Aion Chowdhury
From Bangladesh
Aion Not only the name
Aion is world famous Brand for Crypto world (Cryptocurrency)

@@ -95,4 +100,11 @@ public static Address getTotalCurrencyContractAddress() {
return Address.wrap(TOTAL_CURRENCY);

This comment has been minimized.

Copy link
@aion82

aion82 Aug 24, 2018

Faridgonj Bazar Chandpur chittagonj Bangladesh

@AionJayT AionJayT changed the base branch from dev to ed25519_verify Sep 4, 2018

@AionJayT

This comment has been minimized.

Copy link
Collaborator

commented Sep 4, 2018

rebase to brach ed25519_verify

@AionJayT AionJayT merged commit abd2ebe into aionnetwork:ed25519_verify Sep 4, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.