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

Support for Wallet Test Framework #3737

Merged
merged 8 commits into from
Mar 19, 2024

Conversation

SamWilsn
Copy link
Contributor

@SamWilsn SamWilsn commented Feb 29, 2024

Hey!

I'm one of the developers on the Wallet Test Framework project. As part of our grant from the Ethereum Foundation, we've built some integrations with wallets—Taho included.

This pull request contains the changes we needed to make to automate your wallet: adding a few id attributes, and introducing an attribute to differentiate between signing and sending a transaction. Neither myself nor @gaudren are experienced TypeScript developers, so please forgive any obvious mistakes here.

If you're curious what Taho+WTF looks like:

Peek.2024-02-27.15-49.mp4

And the glue code that makes this work: https://github.com/wallet-test-framework/glue-taho

Assuming you folks are on board, we'd like to eventually discuss integrating WTF with your testing strategy!

Latest build: extension-builds-3737 (as of Tue, 19 Mar 2024 16:33:25 GMT).

Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

This is super cool, thank you so much for the contribution! I have two aspects of the data-broadcast-on-sign piece that I'm grappling with—one is for me to chew on some more, one is a question for you in terms of where we're setting the attribute.

We're likely going to be shipping a small release next week, so I expect we can bundle this in with that!

return (
<section>
<section data-broadcast-on-sign={broadcastOnSign}>
Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that we have to drill the property through so many layers makes me feel weird, but I haven't come up with a better approach off the top of my head. I'm going to sit with it for a little bit :)

Meantime though, is there any chance we can assign this data- attribute at

, or does it have to be available while the skeleton loader displays? I'm asking because in TransactionSignatureDetails we already know that we're dealing with a transaction request, which makes it a better place to handle transaction-request-specific code vs here where we're trying to treat it as a generic signing request.

@SamWilsn
Copy link
Contributor Author

SamWilsn commented Mar 1, 2024

does it have to be available while the skeleton loader displays?

Nope, as long as it eventually appears on the page somewhere. I'll look into that.

@SamWilsn
Copy link
Contributor Author

I moved the attribute as suggested. Let me know if there's anything else you'd like to see changed!

Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

Took a deeper look and actually the way broadcastOnSign was added is perfect, thank you so much! Running the build shortly and then we'll merge and hopefully ship in a release soon.

Comment on lines 109 to +112
Partial<EnrichedEVMTransactionRequest> & {
from: string
network: EVMNetwork
broadcastOnSign: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

Noticing this type is the same as the enrichment type that's being updated here, could reuse that. Just a note to self though 😁

@@ -23,7 +25,7 @@ export default function SharedCurrentAccountInformation({
const icon = areInternalSignersUnlocked ? "unlock" : "lock"
return (
<div className={classNames("account_info_wrap", { hover: showHoverStyle })}>
<span className="account_info_label ellipsis">
<span title={address} className="account_info_label ellipsis">
Copy link
Contributor

Choose a reason for hiding this comment

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

Glorious, thank you for this one 💪

@@ -17,7 +17,7 @@ export default function EIP191Info({
<>
<div className="message">
<div className="message-title">{t("message")}</div>
<div className="light">{`${signingData}`}</div>
<div id="message-content" className="light">{`${signingData}`}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this will cause issues here. Might be better to stay consistent with using either data-testid or id.

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 prefer to just use id if we can. The data-testid stuff is how we automate Coinbase (and I think this is a leftover from when we ported our glue to Taho 😅)

@Shadowfiend
Copy link
Contributor

Ah, missing secrets again. Let me see what I can do here.

@Shadowfiend
Copy link
Contributor

Ok we're just going to see how these do on main heh.

@Shadowfiend Shadowfiend merged commit 4a93eba into tahowallet:main Mar 19, 2024
5 of 6 checks passed
@SamWilsn SamWilsn deleted the wallet-test-framework branch March 20, 2024 04:27
@Shadowfiend Shadowfiend mentioned this pull request Jul 15, 2024
Shadowfiend added a commit that referenced this pull request Oct 5, 2024
## What's Changed
* v0.57.0 by @Shadowfiend in
#3735
* Support for Wallet Test Framework by @SamWilsn in
#3737
* Fix typos by @omahs in
#3724
* Fix a couple of typos from contribution by @Shadowfiend in
#3742
* Move action using node12 to a newer version by @michalinacienciala in
#3722
* Changed the build for the firefox browser by @kogeletey in
#3733
* Remove Goerli testnet from the testnets list by @michalinacienciala in
#3718
* Move autolock timer by @radchukd in
#3743
* Tweaks and creaks: Disable UNS, fix Ledger signing on built-in
non-ETH/MATIC networks by @Shadowfiend in
#3746

## New Contributors
* @SamWilsn made their first contribution in
#3737
* @kogeletey made their first contribution in
#3733
* @radchukd made their first contribution in
#3743

Latest build:
[extension-builds-3747](https://github.com/tahowallet/extension/suites/26032567144/artifacts/1703810792)
(as of Mon, 15 Jul 2024 21:35:33 GMT).
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.

4 participants