Skip to content

feat: Add support for Permissioned Keys#328

Closed
konichuvak wants to merge 6 commits intodydxprotocol:mainfrom
konichuvak:feature/py-auth
Closed

feat: Add support for Permissioned Keys#328
konichuvak wants to merge 6 commits intodydxprotocol:mainfrom
konichuvak:feature/py-auth

Conversation

@konichuvak
Copy link
Copy Markdown

Adds functionality to enable signing transactions using the authenticator flow on the protocol.

Basically following the footsteps of #317 to add the same functionality to the Python client.

@konichuvak konichuvak requested review from a team as code owners February 6, 2025 03:54
konichuvak added 2 commits February 5, 2025 23:04
- bumps v4-proto and grpcio to get protobufs for authenticators
- adds client methods for adding / removing / retrieving authenticators
- adds TxOption parameter to place / cancel methods to allow signing orders on behalf of another wallet
- modifies transaction building flow to include TxOptions
Copy link
Copy Markdown
Contributor

@pnowosie pnowosie left a comment

Choose a reason for hiding this comment

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

@konichuvak Thank you very much for you contribution. 🙌

I'd like to kindly ask you to check the comments below and respond if you think they're make sense.

Also I spotted a formatting issues. Please make sure you run poetry run black . to reformat the code before pushing.

Thanks 🙏

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was not able to run this example. Here is the error message:

grpc._channel._InactiveRpcError: <_InactiveRpcError of RPC that terminated with:
	status = StatusCode.NOT_FOUND
	details = "account dydx18sukah44zfkjndlhcdmhkjnarl2sevhwf894vh not found"

Account dydx18sukah44zfkjndlhcdmhkjnarl2sevhwf894vh doesn't exists on testnet.
Have you succeeded running the example?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Additionally, I would like to see the output when running the example. Could you introduce some print statements? e.g. examples/batch_cancel_example.py

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I am able to run the example with my own accounts on the mainnet.
The example there is just a copy of the JS client example, which does not run for me either btw.

I can parametrize the example such that the user would have to supply their own mainnet keys. Let me know if you have better ideas.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was able to fix the example and run it on testnet.
The reason it might work for you on mainnet is because you used owner's wallet to sign so no authenticator was needed

response = await node.place_order(
wallet,
market.order(

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

glad you made it work in the end!

good, point! that should be a trader_wallet there

return base64.b64decode(value)


def decode_authenticator(config: str, auth_type: str) -> Union[SubAuthenticator, Authenticator]:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this function used anywhere?
I'd prefer not to introduce redundant code.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is very helpful when you want to validate what types of authenticators are added to the account, since it handles nested structure of the sub-authenticators as well performs decoding of the base64 encoded values.

@pnowosie
Copy link
Copy Markdown
Contributor

pnowosie commented Feb 7, 2025

@konichuvak Please ignore my above comments.
We've decided to implement this feature and I will be happy to use your code and modify to our needs.
I will open an subsequent PR and mention this one.

konichuvak added 2 commits February 7, 2025 12:53
- runs linter
- replaces TypedDict with dataclass
- replaces starred imports with explicit ones
@konichuvak
Copy link
Copy Markdown
Author

@konichuvak Please ignore my above comments. We've decided to implement this feature and I will be happy to use your code and modify to our needs. I will open an subsequent PR and mention this one.

No worries, hope you find it useful to have as a reference in your implementation.

@pnowosie
Copy link
Copy Markdown
Contributor

pnowosie commented Feb 8, 2025

No worries, hope you find it useful to have as a reference in your implementation.

I find it very useful, I will make sure to mention your contribution in my PR

pnowosie pushed a commit to pnowosie/v4-clients that referenced this pull request Feb 8, 2025
I only fixed and cleaned previous PR dydxprotocol#328 by konichuvak

Author: pnowosie <pawel@nethermind.io>, konichuvak <konichuvak@proton.me>
pnowosie pushed a commit to pnowosie/v4-clients that referenced this pull request Feb 8, 2025
I only fixed and cleaned previous PR dydxprotocol#328 by konichuvak

Author: pnowosie <pawel@nethermind.io>, konichuvak <konichuvak@proton.me>
@konichuvak konichuvak closed this Feb 8, 2025
UnbornAztecKing pushed a commit that referenced this pull request Mar 4, 2025
This is cleaned & fixed version of #328
Big thanks to the original contributor: @konichuvak

---------

Co-authored-by: konichuvak <konichuvak@proton.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants