Sign Auth Entry API & modal flow #949

aristidesstaffieri · 2023-08-24T15:09:53Z

Ticket: https://stellarorg.atlassian.net/jira/software/c/projects/WAL/boards/36?modal=detail&selectedIssue=WAL-1016&assignee=63bc81730a1b5442166a2f79

This adds a new API signAuthEntry and a matching popup flow for signing auth entries.
Splits signTx and signBlob, so now we have sign-transaction and sign-blob paths instead of a shared one.
Adds buildInvocationTree(can be deleted once the sdk version lands) for crawling and unpacking xdr structures from a root invocation. Adds this to the Transaction component used to render transaction details during signing. Applied feedback to focus more on post sim auth than op.func in order to surface the invocation.

…and related API hooks

…component

… sub invocations

…AuthEntry

aristidesstaffieri · 2023-08-24T15:12:28Z

extension/src/popup/components/signAuthEntry/invocation.ts

+  subInvocations: RootOutput[];
+}
+
+export function buildInvocationTree(root: xdr.SorobanAuthorizedInvocation) {


This can be deleted once the sdk version is out.

aristidesstaffieri · 2023-08-24T15:12:49Z

extension/src/popup/components/signBlob/index.tsx

@@ -10,6 +10,6 @@ interface BlobProps {
 export const Blob = (props: BlobProps) => (
  <Card variant={Card.variant.highlight}>
    <Heading4>Signing data:</Heading4>
-    <div className="Blob">{props.blob}</div>
+    <div className="Blob">{atob(props.blob)}</div>


not sure why we didn't decode the b64 blob before, just oversight.

aristidesstaffieri · 2023-08-24T15:13:41Z

extension/src/popup/components/signTransaction/Operations/index.tsx

+    };
+  }[];
+}) => {
+  // TODO: use getters in signTx to get these correctly


Once we clean up SignTx to better use the xdr class methods, we should be able to clean this code up a bit and use this component in SignAuthEntry as well.

aristidesstaffieri · 2023-08-24T15:14:43Z

extension/src/popup/components/signTransaction/Operations/index.tsx

@@ -783,15 +816,9 @@ export const Operations = ({
                  />
                ))}

-                {scFunc ? (


This applied the feedback to focus more on the post sim auth then the op.func values. They hold similar info but the auth invocation better represents the sim results.

extension/src/popup/views/SignTransaction/index.tsx

aristidesstaffieri · 2023-08-24T15:22:20Z

There is one more refactor here that I haven't made because this PR is already large but I could be convinced to do it here also.

We should split SignTransaction to different render branches for the Transaction vs FeeBumpTransaction cases. Right now, is we let the types come through properly we can uncover some edge cases of our current approach. Also we can refactor this component to better make use of the xdr class methods instead of digging into the xdr class internals in order to improve signTransaction/Operations to more generically cover all transactions cases.

I plan to do this as a separate PR to keep things comprehensible but let me know if you feel strongly that this should happen in this one.

Shaptic

small set of comments, otherwise lgtm from my perspective but my ✔️ is not so much on the actual Freighter side

Shaptic · 2023-08-24T16:53:49Z

@shared/api/external.ts

+      type: EXTERNAL_SERVICE_TYPES.SUBMIT_AUTH_ENTRY,
+    });
+  } catch (e) {
+    console.error(e);


should this also do response.error = e;?

I think it's more appropriate to throw here because that would be an implementation error when the other is a type or API error. Added in 63a77ea but let me know if you disagree.

def up to y'all cuz I'm unfamiliar w/ your error-handling conventions, I just more wanted to call out the distinction between exceptions and the response.error field

extension/src/background/messageListener/popupMessageListener.ts

extension/src/helpers/urls.ts

extension/src/popup/components/signAuthEntry/AuthEntry/index.tsx

extension/src/popup/components/signTransaction/Operations/index.tsx

…handler

piyalbasu · 2023-08-24T17:36:31Z

@shared/api/external.ts

+
+  if (error) {
+    throw error;
+    throw error;


repeated line

whops, thanks for the catch. Fixed in 4b0d05a

piyalbasu · 2023-08-24T17:52:19Z

extension/src/popup/views/AccountHistory/index.tsx

@@ -88,18 +89,20 @@ export const AccountHistory = () => {
  const stellarExpertUrl = getStellarExpertUrl(networkDetails);

  // differentiate between if data is still loading and if no account history results came back from Horizon
+  const shouldLoadToken =
+    isExperimentalModeEnabled || networkDetails.network === NETWORKS.FUTURENET;


isn't this more dependent on isExperimentalModeEnabled? If that flag is true, soroban-client will be used, but it will use stellar-sdk if it's false.

Also, since we're going live on Testnet in a few weeks, maybe we should just key this off of isExperimentalModeEnabled. I'm guessing the Futurenet check is to make sure the data exists (or something like that). Maybe we should gracefully fail if the data isn't found on network

i'm not sure that I understand, how does this toggle the sdk? AFAIk this toggles wether we fetch and add soroban tokens to the account history. The network check is needed because you can be in experimental mode while not being on Futurenet, so as currently implemented it incorrectly shows tokens that are not for the network when on Testnet.

Oh actually - you're right. We're just grabbing the Horizon entry, so we can just use stellar-sdk here. I was thinking of other places in the app where we do: const SDK = isExperimentalModeEnabled ? SorobanSdk : StellarSdk;. You can ignore that first part 😛

but still I think also the second part we cannot do right now because the tokens as we store them today have no relation to any network(we should address this before testnet) so when on Testnet but with experimental mode toggled on, you incorrectly see tokens that are on Futurenet without the network check.

I guess for the second part: I think instead of checking for Futurenet explicitly here, we should adjust the network url that SorobanContext is using. On closer inspection, looks like SorobanContext is hardcoded to use Futurenet. So, in getTokenBalances on ln 162, it's always checking Fututenet.

We should fix that in SorobanContext (but maybe we just do that in a later PR)

This is mostly so when Soroban is running on both Futurenet and Testnet, we have fewer places in the code where we have explicit checks for Futurenet that we have to manually remove

We must have been writing this at the same time. Yeah we should def be able to configure the SorobanContext for other networks before Testnet support is live. We still can't set the network in the SorobanContext to whatever the current network is here to fix this bug because a) the saved tokens have no relation to a network today, b) There is still only futurenet support so this is incorrect today.

There's def a bigger refactor here to support future networks but this is just meant to address the live bug where we show users the wrong tokens when in experimental mode and not on Futurenet. In order to fix this to work for other networks, we also need to refactor how we store tokens to include network details, then change all token related checks to instead map to tokens that are saved for that network.

Cool - yeah, let's just leave this is as-is then, and we can tackle the network stuff later on

but good call out as I hadn't really considered before this that there will be significant work to support tokens on other networks, and it will include the always hairy work of migrating storage schemas.

piyalbasu · 2023-08-24T17:56:40Z

extension/src/popup/views/SignBlob/index.tsx

+    }
+  }, [startedHwSign, hwStatus]);
+
+  useEffect(() => {


I think something you did a good job of in the PR to add sign-blob was put this "auto-select account/set current account" logic in one place and pass it to different views. I think you should do the same here so we're not repeating this logic in all the 3 sign-X views

Hmm yeah I chose the repetition because they no longer share a common ancestor where it would make sense to hold common logic. I think the solution now would have to be a wrapper component that does the shared stuff and passes it to a generic child prop. Let me give that a shot then.

Might also work as a custom hook?

Really want to avoid the repetition bc the logic is a little hairy

made a new hook that worked pretty nicely for this in 47ea4f9

…supported in ledger API

@shared/constants/stellar.ts

extension/src/popup/views/SignAuthEntry/index.tsx

piyalbasu · 2023-08-28T17:11:04Z

extension/src/popup/helpers/useSetupSigningFlow.ts

+} from "popup/ducks/transactionSubmission";
+
+export function useSetupSigningFlow(
+  dispatch: AppDispatch,


do you need to pass dispatch? or can you just instantiate a new one with useDispatch? Might make the API a little nicer to have one fewer param

yeah 100%, moved in 06295ca

aristidesstaffieri added 10 commits August 21, 2023 16:33

adds new path for signing auth entries, adds SignAuthEntry component …

6b98f9a

…and related API hooks

Adds basic signing component for auth entry sign flow

a6bad1e

Adds new external api signAuthEntry, adds docs, adds sign auth entry …

5af3394

…component

Added translations

380530a

adds buildInvocationTree to render formatted json for invocations and…

a06003e

… sub invocations

use correct key in responseQueue handler for signAuthEntry, clean up …

c589044

…AuthEntry

splits SignTx and SignBlob, makes new route for sign-blob

db78b9f

use buildInvocationTree in transaction render for signTx

7f7eb56

Added translations

3fb4eff

Added translations

4432d60

aristidesstaffieri self-assigned this Aug 24, 2023

remove accidental tarball commit

2efbb74

aristidesstaffieri commented Aug 24, 2023

View reviewed changes

extension/src/popup/views/SignTransaction/index.tsx Show resolved Hide resolved

aristidesstaffieri requested review from piyalbasu and Shaptic August 24, 2023 15:22

Shaptic reviewed Aug 24, 2023

View reviewed changes

adds missing loading states, fixes missing error handling in message …

63a77ea

…handler

piyalbasu reviewed Aug 24, 2023

View reviewed changes

remove extra line

4b0d05a

piyalbasu reviewed Aug 24, 2023

View reviewed changes

aristidesstaffieri added 4 commits August 25, 2023 10:51

refactors signing flows to use a common helper hook

47ea4f9

dont allow HW signing on sign blob or sign auth entry, not currently …

433a42d

…supported in ledger API

Added translations

1fdbabf

Added translations

d0b7bdf

aristidesstaffieri requested a review from piyalbasu August 25, 2023 20:14

Shaptic reviewed Aug 25, 2023

View reviewed changes

@shared/constants/stellar.ts Outdated Show resolved Hide resolved

extension/src/popup/views/SignAuthEntry/index.tsx Outdated Show resolved Hide resolved

aristidesstaffieri added 3 commits August 25, 2023 15:06

updates HW wallet not supported text

d1f4093

Added translations

860e293

refactors SorobanContext to accoutn for missing rpc urls

e03372a

piyalbasu reviewed Aug 28, 2023

View reviewed changes

piyalbasu approved these changes Aug 28, 2023

View reviewed changes

moves dispatch to be in shared signing hook

06295ca

aristidesstaffieri merged commit 15d138e into release/5.5.0 Aug 28, 2023
2 checks passed

aristidesstaffieri deleted the feature/sign-auth-entry branch August 28, 2023 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sign Auth Entry API & modal flow #949

Sign Auth Entry API & modal flow #949

aristidesstaffieri commented Aug 24, 2023

aristidesstaffieri Aug 24, 2023

aristidesstaffieri Aug 24, 2023

aristidesstaffieri Aug 24, 2023

aristidesstaffieri Aug 24, 2023

aristidesstaffieri commented Aug 24, 2023

Shaptic left a comment

Shaptic Aug 24, 2023

aristidesstaffieri Aug 24, 2023

Shaptic Aug 24, 2023

piyalbasu Aug 24, 2023

aristidesstaffieri Aug 24, 2023

piyalbasu Aug 24, 2023

aristidesstaffieri Aug 24, 2023

piyalbasu Aug 24, 2023

aristidesstaffieri Aug 24, 2023

piyalbasu Aug 24, 2023

aristidesstaffieri Aug 24, 2023

aristidesstaffieri Aug 24, 2023

piyalbasu Aug 24, 2023

aristidesstaffieri Aug 24, 2023

piyalbasu Aug 24, 2023

aristidesstaffieri Aug 24, 2023

piyalbasu Aug 24, 2023

aristidesstaffieri Aug 25, 2023

piyalbasu Aug 28, 2023

aristidesstaffieri Aug 28, 2023

Sign Auth Entry API & modal flow #949

Sign Auth Entry API & modal flow #949

Conversation

aristidesstaffieri commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aristidesstaffieri commented Aug 24, 2023

Shaptic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment