Skip to content

fix memo required check; rm horizon check for destination #2481

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.

Sign up for GitHub

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

Jump to bottom
Merged
piyalbasu merged 3 commits into from
Jan 8, 2026
Merged

fix memo required check; rm horizon check for destination #2481

piyalbasu merged 3 commits into from
Jan 8, 2026
+130 −77

Conversation

@piyalbasu
Copy link
Contributor

@piyalbasu piyalbasu commented Jan 2, 2026

Fixing a couple of bugs on the release branch:

  1. There was a race condition with fetching "memo required" accounts during the Send flow. I've fixed that while also updating a related incorrect test
  2. Removes a superfluous call to Horizon when showing a muxed account in Account History. Horizon returns the base G address when you send to/receive from a muxed account/ We need the XDR to show the user the muxed address. The fix that was merged called Horizon to get the payment's xdr by tx hash, but this is actually an unnecessary API call. We already have the XDR; no need to refetch. I should have noticed this during the PR review process, but only caught this while QA'ing.
  3. There was a duplicate Review Send button in the Send flow. We had a few big PR's all landing at the same time and when I rebased, I missed this

aristidesstaffieri
aristidesstaffieri reviewed Jan 2, 2026
View reviewed changes
extension/src/popup/helpers/useValidateTransactionMemo.ts
setIsMemoMissing(true);
setIsValidatingMemo(false);

if (!shouldValidateMemo || !xdr || !networkDetails) {
Copy link
Contributor

@aristidesstaffieri aristidesstaffieri Jan 2, 2026

Choose a reason for hiding this comment

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

wdyt about adding a unit test that asserts this condition?

Copy link
Contributor Author

@piyalbasu piyalbasu Jan 2, 2026

Choose a reason for hiding this comment

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

Sure I can do that. Fwiw, the e2e tests pick this up (they were failing before), but no harm in adding a unit test 👍

@piyalbasu piyalbasu merged commit 641f9c9 into release/5.37.0 Jan 8, 2026
2 of 5 checks passed
@piyalbasu piyalbasu deleted the bugfix/release-fixes branch January 8, 2026 01:45
piyalbasu added a commit that referenced this pull request Jan 8, 2026
@piyalbasu
fix memo required check; rm horizon check for destination (#2481)
44b8243 
* fix memo required check; rm horizon check for destination

* comment out test that will not pass

* add unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aristidesstaffieri aristidesstaffieri aristidesstaffieri left review comments

@CassioMG CassioMG CassioMG approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@piyalbasu @CassioMG @aristidesstaffieri