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

Sign a tx from link opened via CLI #987

Merged
merged 13 commits into from
Sep 11, 2024

Conversation

elizabethengelman
Copy link
Contributor

@elizabethengelman elizabethengelman commented Aug 27, 2024

Companion PR: stellar/stellar-cli#1579

closes #928

This PR is now using an approach that CLI can send a req to a url with the query params networkPassphrase and xdr, and expect lab to parse those args. This was so that the CLI sign_with_lab functionality could remain generic, and not be tied to Lab's implementation.

To test, without using the CLI, go to the URL that the CLI produces:
http://laboratory-pr987.previews.kube001.services.stellar-ops.com/transaction/cli-sign?networkPassphrase=Test+SDF+Network+%3B+September+2015&xdr=AAAAAgAAAADcOHnq5sGLOngOCEMyLqqn5CvFV2HGbOSjJAIzhqBdkAAAAGQAEb7UAAAAAQAAAAAAAAAAAAAAAQAAAAAAAAAYAAAAAQAAAAAAAAAAAAAAANw4eermwYs6eA4IQzIuqqfkK8VXYcZs5KMkAjOGoF2Q04SY%2FPMseMCgfbEFkX5gTr9qD%2Fd0mvD9H3acFOz%2FX5oAAAAALtGgcgsAibk5q8VOLY2R1G1NdGX7fLzlz7iBFBy64JIAAAAAAAAAAAAAAAA%3D


old notes:
This is a work in progress that I'd love some eyes on to help me think through the approach. For now I am just focusing on allowing the CLI to open Lab with the tx XDR already imported, and ready to sign.

  • To sign a tx on the CLI side, we have easy access to the rpc-url and the network-password.

  • To sign a tx in Lab, we need a Network object:

     Network = {
        id: NetworkType;
        label: string;
        horizonUrl: string;
        rpcUrl: string;
        passphrase: string;
    }
    
  • Instead of adding additional network information to CLI, my current approach is to pass the tx xdr, and the configured network_passphrase from the CLI to Lab via query string params. We should be able to look up the Network by the passphrase based on the NetworkOptions.

  • I first tried to just pass the network passphrase in the query string like this: http://localhost:3000/transaction/sign?$=network$passphrase=Test SDF Network /; September 2015;&transaction$sign$activeView=overview&importXdr=AAAAAgAAAAC...=;;

  • But, if we don’t pass the network id in the /transaction/sign query string, Lab will use a default network that is not necessarily the one that the user configured with their CLI command. This behavior makes sense to me, but I wasn’t sure of the best way to get around the auto network selection, and instead use the network passphrases passed in to lookup which network to use.

    • The network selector functionality happens before we get to the transaction/sign page. So by the time we get to the sign tx page, the query string & store have been updated to include the newly chosen network. So we no longer have the network passphrase information the user sent from the CLI to look up the correct network.
    • example
      • when we visit /transaction/sign?$=network$passphrase=Test SDF Network /; September 2015;&transaction$sign$activeView=overview&importXdr=AAAAAgAAAAC...=;;
      • the network selector will update it to /transaction/sign?$=network$id=futurenet&label=Futurenet&horizonUrl=https:////horizon-futurenet.stellar.org&rpcUrl=https:////rpc-futurenet.stellar.org&passphrase=Test%20SDF%20Future%20Network%20/;%20October%202022;&transaction$sign$activeView=overview&importXdr=AAAAAgAAAAC...0=;;
  • A couple ideas I had to get around this:

    • Figure out what that assumed network id should be in the CLI, based on the network passphrase the user has configured and pass that along in the Lab request query string. But I wonder if it makes sense to do this in Lab instead since we already have the full network settings to do the look up in NetworkOptions
    • Add another query param when sending the request to /transaction/sign that wouldn’t get overwritten in the NetworkSelector, but this felt like it wasn’t the right call. We already have the info we need.
    • Have an intermediary page /transaction/cli-sign that has a manual Network selector, and then redirects to `/transaction/sign.

The last option is what I am currently doing in this PR, though it doesn't seem ideal. Curious if anyone has any thoughts on how to handle this. I haven't used zustand-querystring before, so I may be missing some option with that lib to handle this better.

Sidenote: I also noticed that some of the weird URL escaping that zustand-querystring may be fixed in more recent release, but that seems like something we can address in follow up PRs.

@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

@elizabethengelman elizabethengelman changed the title Add a transaction cli-sign page Sign a tx from link opened via CLI Aug 29, 2024
@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

1 similar comment
@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

@stellar-jenkins
Copy link

import { useEffect } from "react";

// Component to set Network and Xdr if only password is given in query string
export const NetworkByPasswordProvider = ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd love some feedback/other ideas on the name of this component.

@elizabethengelman elizabethengelman marked this pull request as ready for review September 3, 2024 18:28
@stellar-jenkins
Copy link

1 similar comment
@stellar-jenkins
Copy link

@janewang janewang requested a review from quietbits September 3, 2024 22:42
@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

Comment on lines 8 to 10
import { NetworkByPasswordProvider } from "@/components/NetworkByPasswordProvider";
import "@/styles/globals.scss";
import "@stellar/design-system/build/styles.min.css";
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the Design System import before the global.css to make sure the global overrides work.

Comment on lines 1 to 18
"use client";

import { Routes } from "@/constants/routes";
import { useStore } from "@/store/useStore";
import { Loader } from "@stellar/design-system";
import { useRouter } from "next/navigation";
import { useEffect } from "react";

export default function CliSignTransaction() {
const { network, transaction } = useStore();
const { sign } = transaction;
const router = useRouter();
useEffect(() => {
router.push(Routes.SIGN_TRANSACTION);
}, [sign.importXdr, network, router]);

return <Loader />;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this idea of using the route to do the redirect. 🙌 Do you think we could move NetworkByPasswordProvider logic here and remove it completely from src/app/layout.tsx. That way, everything related to CLI sign transaction would be handled in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, great idea! I'll get an update pushed shortly

@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

@Ifropc
Copy link

Ifropc commented Sep 6, 2024

Functionally and code-vise LGTM!

src/app/(sidebar)/transaction/cli-sign/page.tsx Outdated Show resolved Hide resolved
}

router.push(Routes.SIGN_TRANSACTION);
}, [sign.importXdr, network, router, searchParams, updateIsDynamicNetworkSelect, selectNetwork, transaction]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally, we shouldn't use objects as dependencies (network, searchParams, transaction, for example) because they will trigger unwanted re-renders (use strings and numbers instead). It might not matter much in this case because users won't stay on this component for long, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! I think my IDE was being a little too generous with the deps... I'll update this shortly.

@stellar-jenkins
Copy link

1 similar comment
@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

@quietbits quietbits merged commit c94420e into stellar:master Sep 11, 2024
2 checks passed
@elizabethengelman elizabethengelman deleted the feat/sign-tx-from-cli branch September 11, 2024 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

CLI: import transaction from URL to sign
4 participants