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

Do not show actions for untrusted regulated assets #194

Merged
merged 2 commits into from
Jun 25, 2021

Conversation

quietbits
Copy link
Contributor

@quietbits quietbits commented Jun 24, 2021

image

@stellar-jenkins
Copy link

Copy link
Contributor

@marcelosalloum marcelosalloum left a comment

Choose a reason for hiding this comment

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

PR looks good! I just have a question below

Comment on lines +60 to +62
if (isUntrusted && asset.supportedActions?.sep8) {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want the SEPs to be available before adding a trustline to a non-regulated asset?

I'm wondering if this change is desired only for regulated assets or if for all assets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some SEPs that can work without a trustline (it is created during deposit, for example). If SEP-8 is not supported for that asset, we would show everything as it was before.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, got it. Now I can see SEP-6 and SEP-24 actions are supported when there's no trustline: https://stellar-demo-wallet-pr194.previews.kube001.services.stellar-ops.com/account?secretKey=SCIAIFDX7QYORHOWZNNU24DXQXTNP2AABHGNRJ5WPZ6AC6VXXR3VCTHT&untrustedAssets=MULT%3AGDLD3SOLYJTBEAK5IU4LDS44UMBND262IXPJB3LDHXOZ3S2QQRD5FSMM

I think this behaviour should remain the same across SEP-8 and non-SEP-8 assets. There could be an anchor implementing SEP-6, SEP-24 and SEP-8 for the same asset. If that's the case, we should still show the dropdown with SEP-6 and SEP-24 for that asset even if that's a regulated asset (SEP-8), right?

I think the only change we need is to add the !isUntrusted && condition before rendering the SEP-8 send option, i.e. update:

            {asset.supportedActions?.sep8 && (
              <option value={AssetActionId.SEP8_SEND_PAYMENT}>
                SEP-8 Send
              </option>
            )}

to

            {!isUntrusted && asset.supportedActions?.sep8 && (
              <option value={AssetActionId.SEP8_SEND_PAYMENT}>
                SEP-8 Send
              </option>
            )}

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Sure, we can add that for clarity. Line 60 check should prevent it from being an untrusted asset for SEP-8 at that point, I think.

@stellar-jenkins
Copy link

Copy link
Contributor

@marcelosalloum marcelosalloum left a comment

Choose a reason for hiding this comment

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

LGTM!

I created an issue to normalize how we display the dropdown options before adding the trustline: #195

@quietbits quietbits merged commit b0c9c5e into release/1.2.0 Jun 25, 2021
@quietbits quietbits deleted the il-sep-8-untrusted branch June 25, 2021 15:52
marcelosalloum pushed a commit that referenced this pull request Jul 19, 2021
* Do not show actions for untrusted regulated assets

* Add extra check
marcelosalloum pushed a commit that referenced this pull request Jul 19, 2021
* Do not show actions for untrusted regulated assets

* Add extra check
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.

None yet

4 participants