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

Implement 0x gasless swap approve method #331

Closed
wants to merge 2 commits into from

Conversation

samholmes
Copy link
Contributor

@samholmes samholmes commented Jun 21, 2024

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Description

none

@samholmes samholmes force-pushed the sam/0x-gassless-swap-approve branch 2 times, most recently from 26bf47c to 9e0b084 Compare June 24, 2024 21:47
Copy link
Contributor

@swansontec swansontec left a comment

Choose a reason for hiding this comment

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

Just some minor stuff. This looks good overall.

// Some values may be updated later when the transaction is
// updated from queries to the network.
const transaction: EdgeTransaction = {
txid: apiSwapStatus.transactions[0].hash.slice(2), // Remove '0x'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer .replace('0x', '') instead of .slice(2), in case they ever change their format. Also, can we alphabetize this section?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see the slice is moot now. Still, sorting might be nice.

const transaction: EdgeTransaction = {
txid: apiSwapStatus.transactions[0].hash.slice(2), // Remove '0x'
date: Date.now(),
currencyCode: fromCurrencyCode,
Copy link
Contributor

@swansontec swansontec Jun 24, 2024

Choose a reason for hiding this comment

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

Interesting. This whole repo is using edge-core-js v2, whereas in the new core the currencyCode has been replaced with tokenId in all cases. That's why the network fee needed a currencyCode in the previous commits.

We don't need to do anything here, since upgrading the core is independent of adding this plugin, but it's something to keep in mind for the future. We may want to schedule a core upgrade in the near-ish future so future plugins can be cleaner.

@samholmes samholmes force-pushed the sam/0x-gassless-swap-approve branch from 9e0b084 to 367ebe7 Compare June 25, 2024 21:40
@samholmes
Copy link
Contributor Author

Closed in favor of the final PR: #334

@samholmes samholmes closed this Jun 25, 2024
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.

2 participants