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

Fix address derivation from pubKey #10

Closed
wants to merge 5 commits into from

Conversation

AyushBherwani1998
Copy link

@AyushBherwani1998 AyushBherwani1998 commented Apr 11, 2024

Use uncompressed pubKey to derive the address.

@AyushBherwani1998
Copy link
Author

There's one issue with implementation, we can't throw the error. So, we have to assume, it's always best case when deriving the address. The error can't be thrown because we have inherit EthereumAccountProtocol

@AyushBherwani1998 AyushBherwani1998 changed the title Fix address generation Fix address derviation from pubKey Apr 11, 2024
@AyushBherwani1998 AyushBherwani1998 changed the title Fix address derviation from pubKey Fix address derivation from pubKey Apr 11, 2024
@metalurgical
Copy link
Contributor

metalurgical commented Apr 25, 2024

@AyushBherwani1998

If we have to do this there like you're suggesting there is a larger design problem. The reason it is not throwable there is that it should always be valid by the time it reaches that point, empty is not a valid output either.

Please see the original implementation for where the error for this should be thrown and how it is guaranteed to be valid thereafter:
https://github.com/tkey/web3-swift-mpc-provider/blob/main/Sources/Web3SwiftMpcProvider/EthereumTssAccount.swift#L32-L39

@AyushBherwani1998
Copy link
Author

AyushBherwani1998 commented Apr 25, 2024

@metalurgical the problem is not whether I can throw the error, the problem is that the throwing error doesn't conform to the address definition from EthereumProtocol. To throw an error I need to use the throw keyword to change the definition of the address which doesn't conform to EthereumProtocol.

@metalurgical
Copy link
Contributor

metalurgical commented Apr 25, 2024

@metalurgical the problem is not whether I can throw the error, the problem is that the throwing error doesn't conform to the address definition from EthereumProtocol. To throw an error I need to use the throw keyword to change the definition of the address which doesn't conform to EthereumProtocol.

Yes, exactly, the address should have already been validated previously and assigned here, not trying to calculate what the address is here. Like I said, it is a larger design problem with the signing kit.

@AyushBherwani1998
Copy link
Author

@metalurgical I'm thinking to add the isCompressed flag in getTssPubKey() of core-kit swift. By default, we can keep it true so the existing functionality doesn't break. And if the flag is false we return the uncompressed key. I think this will solve the issue. WDYT?

@metalurgical
Copy link
Contributor

@metalurgical I'm thinking to add the isCompressed flag in getTssPubKey() of core-kit swift. By default, we can keep it true so the existing functionality doesn't break. And if the flag is false we return the uncompressed key. I think this will solve the issue. WDYT?

It still won't get rid of the try.

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.

2 participants