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

Add origination operation page #1285

Merged
merged 1 commit into from
May 24, 2024

Conversation

OKendigelyan
Copy link
Contributor

@OKendigelyan OKendigelyan commented May 23, 2024

Proposed changes

Task link

Types of changes

  • Bugfix
  • New feature
  • Refactor
  • Breaking change
  • UI fix

Steps to reproduce

Screenshots

Add the screenshots of how the app used to look like and how it looks now

image

Checklist

  • Tests that prove my fix is effective or that my feature works have been added
  • Documentation has been added (if appropriate)
  • Screenshots are added (if any UI changes have been made)
  • All TODOs have a corresponding task created (and the link is attached to it)

@OKendigelyan OKendigelyan self-assigned this May 23, 2024
@OKendigelyan OKendigelyan force-pushed the add-support-for-origination-operations branch from 8bd28c6 to ba41993 Compare May 23, 2024 14:48
@OKendigelyan OKendigelyan marked this pull request as draft May 23, 2024 14:48
@OKendigelyan OKendigelyan force-pushed the add-support-for-origination-operations branch from ba41993 to e0b3caa Compare May 23, 2024 15:48
@OKendigelyan OKendigelyan marked this pull request as ready for review May 23, 2024 15:48
import { executeOperations, makeToolkit } from "../../../utils/tezos";
import { SuccessStep } from "../SuccessStep";

const user = userEvent.setup();
Copy link
Contributor

Choose a reason for hiding this comment

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

it's better to do inside the tests according to this

expect(screen.getByText(prettyTezAmount(fee))).toBeVisible();
});

it("uses correct network", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's enough to check that when you click the button executeOperations gets the correct payload, and the WalletClient too.
this test was written in another component just to make sure that the network selection works correctly, no need to copy it here

<AccordionIcon />
</AccordionButton>
<AccordionPanel overflowY="auto" maxHeight="300px">
<JsValueWrap value={(operation.operations[0] as ContractOrigination).code} />
Copy link
Contributor

Choose a reason for hiding this comment

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

just do

const {code, storage} = operation.operations[0] as ContractOrigination

above

src/utils/beacon/useHandleBeaconMessage.tsx Show resolved Hide resolved
@OKendigelyan OKendigelyan force-pushed the add-support-for-origination-operations branch from e0b3caa to 8a60fd2 Compare May 24, 2024 07:46
Copy link

github-actions bot commented May 24, 2024

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements 89.64% 3634/4054
🟢 Branches 81.89% 1262/1541
🟢 Functions 88.01% 1108/1259
🟢 Lines 89.57% 3435/3835

Test suite run success

1402 tests passing in 183 suites.

Report generated by 🧪jest coverage report action from e168d7a

@OKendigelyan OKendigelyan force-pushed the add-support-for-origination-operations branch from 8a60fd2 to 6ba0dee Compare May 24, 2024 09:13
@OKendigelyan OKendigelyan force-pushed the add-support-for-origination-operations branch from 6ba0dee to 567431d Compare May 24, 2024 09:13
Copy link
Contributor

@asiia-trilitech asiia-trilitech left a comment

Choose a reason for hiding this comment

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

LGTM - great job Oleg!

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the rest of the modals has "Page" suffix - should we maybe rename them to be consistent?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the tests 🥇


it("passes correct payload to sign handler", async () => {
const user = userEvent.setup();
const testToolkit = "testToolkit" as unknown as TezosToolkit;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to avoid casting to unknown here somehow?

@OKendigelyan OKendigelyan force-pushed the add-support-for-origination-operations branch 3 times, most recently from c3ea845 to da4c206 Compare May 24, 2024 11:57
@OKendigelyan OKendigelyan changed the title Add origination operation modal Add origination operation page May 24, 2024
@OKendigelyan OKendigelyan force-pushed the add-support-for-origination-operations branch from da4c206 to e168d7a Compare May 24, 2024 12:16
@OKendigelyan OKendigelyan merged commit 9241e10 into main May 24, 2024
4 checks passed
@OKendigelyan OKendigelyan deleted the add-support-for-origination-operations branch May 24, 2024 15:03
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.

3 participants