XRPLF / xrpl-py

A Python library to interact with the XRP Ledger (XRPL) blockchain
ISC License
144 stars 83 forks source link

Make `sign` synchronous #598

Closed JST5000 closed 1 year ago

JST5000 commented 1 year ago

High Level Overview of Change

Sign is currently async, but doesn't need to be (and it's bad practice to do online operations during signing)

Context of Change

Signing should happen offline (otherwise it may give the impression that keys are being leaked) - so having an asynchronous sign function is odd. The only reason for it is a single edge-case sanity check, which can be moved elsewhere to achieve similar coverage without clouding the purpose of sign.

Type of Change

Test Plan

CI Passes

JST5000 commented 1 year ago

Currently sign was in both asyncio and sync sides of the library. Now that it's just synchronous - I'm not sure whether to remove the asyncio side or not.

I feel like forcing people to change their imports is an annoying change and somewhat hard to debug - so I'm inclined to just leave the functions where they are. Thoughts?