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

Ups 889 robo wallet can send solana token lockup transactions #17

Merged

Conversation

makarychev
Copy link
Collaborator

The goal is to implement integration for TokenLockup Solana program instructions and corresponding transaction types:

  • FundReleaseSchedule
  • BatchFundReleaseSchedule

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ This pull request was sent to the PullRequest network.


Check the status or cancel PullRequest code review here.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest Breakdown

Reviewable lines of change

+ 2,156
- 248

77% JavaScript
23% JavaScript (tests)
<1% JSON

Generated lines of change

+ 1,148
- 1,515

Type of change

Feature - These changes are adding a new feature or improvement to existing code.
1 Message
⚠️ Due to its size, this pull request will likely have a little longer turnaround time and will probably require multiple passes from our reviewers.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

As always, fantastic implementation of your test cases and fixtures! I will always commend you and your team with how you follow TDD and ensure as many unit tests can be created to not only have good code coverage but also scenarios that wouldn't be caught by a test coverage tool (e.g. in tests/SolanaTxValidator.isTransactionValid.test.js for test: for valid blockchainTransaction, you confirmed that markAs, error, and switchHWToManualMode are undefined. So in case another developer were to change one of these values, so they are populated, the test can properly catch it for this particular scenario).

Also, great refactor to move the validators into their own files! Makes it much easier to read the Blockchain files and easier to test the validation files in their own bubble.

As I mentioned in my comment for TokenLockupHelper.mjs, an object validation library would save some time on manually checking your data. Especially since typescript isn't being used, I saw a decent amount of type checking explicitly which can be resolved with an object validator. I suggested yup as a good library, but there's other libraries that can also suffice.

Let me know if you have any questions or comments about what I wrote, but other than that, this PR looks good to me!

Image of Thomas Morris Thomas Morris


Was this helpful? Yes | No

Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.

@makarychev makarychev force-pushed the UPS-889-robo-wallet-can-send-solana-token-lockup-transactions branch from 81538a4 to bf20fae Compare February 18, 2022 12:14
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest reviewed the updates made to #17 up until the latest commit (fc6b8f4). No further issues were found.

Reviewed by:

Image of Thomas Morris Thomas Morris

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest reviewed the updates made to #17 up until the latest commit (08a2080). No further issues were found.

Reviewed by:

Image of Thomas Morris Thomas Morris

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard.

@Alexey1100 Alexey1100 merged commit d26140d into main Mar 8, 2022
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