feat: eth_signTransaction and personal_sign components

lukso-network / universalprofile-test-dapp

Testing dApps for ERC725 and Universal Profiles.

https://up-test-dapp.lukso.tech/

17 stars 14 forks source link

Closed JeneaVranceanu closed 5 months ago

JeneaVranceanu commented 1 year ago

Added support for eth_signTransaction and personal_sign.

personal_sign and eth_sign are now one component. The returned value must be the same as for eth_sign.
eth_signTransaction - visually it's a combination of eth_sendTransaction and eth_sign. This component sends a transaction to sign and allows for:
- signer address recovery (same as eth_sign component);
- signature validation through EIP1271.isValidSignature (same as eth_sign component);
- checking if the signer has SIGN permission (just decided to add);

richtera commented 1 year ago

Curious but should we upgrade the sign with ethereum stuff? I had looked at that before but it seemed to not match signatures somewhere

JeneaVranceanu commented 1 year ago

it seemed to not match signatures somewhere

@richtera If so then we need to update it. I'll open a separate task to double-check that SIWE implementation matches the standard.

github-actions[bot] commented 1 year ago

JeneaVranceanu commented 1 year ago

Sorry for the long waiting. Had more issues with tests. Now it's fixed and ready to be re-reviewed and potentially merged.

JeneaVranceanu commented 11 months ago

Sign and Personal Signare very similar, can we merge it into one? Just add another checkbox underSign in with Ethereum`.

I'll need to re-review this PR but yes, we can merge them into one.

Hugoo commented 7 months ago

I guess this is stale - shall we close this one? @JeneaVranceanu

JeneaVranceanu commented 7 months ago

I guess this is stale - shall we close this one? @JeneaVranceanu

I'll check and let you know.

franckwei commented 6 months ago

@JeneaVranceanu FYI this PR has been opened since last summer, should we merge?

JeneaVranceanu commented 6 months ago

@JeneaVranceanu FYI this PR has been opened since last summer, should we merge?

I'm moving the task to testing. I think we will merge tomorrow. It's kind of the lowest priority 😅