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

fix: missing rewardId #8916

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

fix: missing rewardId #8916

wants to merge 1 commit into from

Conversation

0xApotheosis
Copy link
Member

@0xApotheosis 0xApotheosis commented Feb 21, 2025

Description

Adds typing to the query params of the routes in position details, which exposes the actual issue: rewardAddress (and contractAddress) is not guaranteed to be defined.

Guard against that so we don't crash:

Screenshot 2025-02-21 at 14 31 56

Side note, we should be typing all of these stringified query params, but the DeFi section is already a mess of duplicate spaghetti and it's probably best to just burn it all.

Issue (if applicable)

Closes #8910

Risk

Low

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Interaction with "Each" positions.

Testing

Engineering

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

This branch, no "Manage" button:

Screenshot 2025-02-21 at 14 20 59

Production, manage button of death:

Screenshot 2025-02-21 at 14 31 04

@0xApotheosis 0xApotheosis requested a review from a team as a code owner February 21, 2025 03:32
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Tested against develop (left), while I'm all for type safety, I don't think this guy is what we want at runtime, IIRC rewardId is actually expected to be optional (i.e all opportunities may not yield rewards), though as you've mentioned in the PR body, hard to confirm without proper types!

  • ATOM manage is now hasta la vista 🚫
image
  • I don't think removing "Manage" for RFOX altogether is what we want. The whole reason RFOX it's not working with "By Asset" is probably the same as other things that were/are broken there: we never gave it love until it became the main and only view in feat: the big short #8876. With that being said, "Manage" should work the same way it did in "By Provider" i.e linking to RFOX, see below:

https://github.com/shapeshift/web/pull/7373/files#diff-a64829c641769089ca72cc06804147035ee12f34d7544ed437fe5b4b6e387248R86-R89

@gomesalexandre
Copy link
Contributor

Relevant comment: #8917 (comment)

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.

Accessing rFOX from earn page is borked
2 participants