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.

Sign up for GitHub

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

Jump to bottom

refactor: remove native token #2280

Merged
merged 9 commits into from
Sep 15, 2023
Merged

refactor: remove native token #2280

merged 9 commits into from
Sep 15, 2023

Conversation

LHerskind
Copy link
Contributor

Fixes #2277.

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

  • If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
  • I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
  • Every change is related to the PR description.
  • I have linked this pull request to relevant issues (if any exist).

@LHerskind LHerskind force-pushed the lh/lending_replace_native_token branch from d8ca053 to 464f39b Compare September 13, 2023 22:02
Base automatically changed from lh/lending_replace_native_token to master September 13, 2023 22:25
@LHerskind LHerskind force-pushed the lh/removing_native_token branch 2 times, most recently from ebf32b9 to 2b379f7 Compare September 14, 2023 15:01
@LHerskind LHerskind marked this pull request as ready for review September 14, 2023 15:01
Maddiaa0
Maddiaa0 reviewed Sep 14, 2023
View reviewed changes
Copy link
Member

@Maddiaa0 Maddiaa0 left a comment

Choose a reason for hiding this comment

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

Not fully sure what the errors are around unencrypted logs but it would be good to have an attached issue describing, other than that lgtm

yarn-project/end-to-end/src/guides/dapp_testing.test.ts Outdated
const secretHash = await computeMessageSecretHash(secret);
expect((await token.methods.mint_private(20n, secretHash).send().wait()).status).toEqual(TxStatus.MINED);
expect(
(await token.methods.redeem_shield({ address: recipient.getAddress() }, 20n, secret).send().wait()).status,
Copy link
Member

Choose a reason for hiding this comment

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

could use the helper from the previous pr here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The waitForSuccess? Was not used here due to this being more referred to in the docs so seemed annoying if you see it in docs and it is not an inbuilt function 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Ah i see good enough reason!

yarn-project/end-to-end/src/guides/dapp_testing.test.ts
expect(
(await token.methods.redeem_shield({ address: recipient.getAddress() }, 20n, secret).send().wait()).status,
).toEqual(TxStatus.MINED);
expect(await token.methods.balance_of_private({ address: recipient.getAddress() }).view()).toEqual(20n);
Copy link
Member

Choose a reason for hiding this comment

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

As this is being used in the example we should probably limit the level of chaining in the calls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ye, just wasted a lot of space otherwise, but maybe fine to include the waitForSuccess or just do the wait and ignore the status as we are doing in quite a few places anyway 🤷

yarn-project/end-to-end/src/guides/dapp_testing.test.ts
expect(toBigIntBE(balance!)).toEqual(100n);
// docs:end:public-storage
});

it('checks unencrypted logs', async () => {
it('checks unencrypted logs, [Kinda broken with current implementation]', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

issue to fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #2306 for it, will update

rahul-kothari
rahul-kothari reviewed Sep 15, 2023
View reviewed changes
yarn-project/end-to-end/src/guides/dapp_testing.test.ts Outdated

beforeAll(async () => {
// docs:start:in-proc-sandbox
({ rpcServer: rpc, stop } = await createSandbox());
// docs:end:in-proc-sandbox
owner = await createAccount(rpc);
recipient = await createAccount(rpc);
token = await PrivateTokenContract.deploy(owner, 100n, owner.getAddress()).send().deployed();
token = await TokenContract.deploy(owner).send().deployed();
expect((await token.methods._initialize({ address: owner.getAddress() }).send().wait()).status).toEqual(
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit - I prefer the old school method where:

 const tx = await token.methods._initialize({ address: owner.getAddress() }).send()
const receipt = tx.wait
expect(....)

Since this is a guide, might be good to reduce chaining

@LHerskind LHerskind enabled auto-merge (squash) September 15, 2023 11:55
@LHerskind LHerskind merged commit 4032d01 into master Sep 15, 2023
2 checks passed
@LHerskind LHerskind deleted the lh/removing_native_token branch September 15, 2023 11:59
@AztecBot AztecBot mentioned this pull request Sep 15, 2023
Merged
kevaundray pushed a commit that referenced this pull request Sep 15, 2023
@AztecBot
chore(master): Release 0.7.4 (#2320)
01bbdaa 
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.7.4</summary>

##
[0.7.4](aztec-packages-v0.7.3...aztec-packages-v0.7.4)
(2023-09-15)


### Features

* Elliptic Curve Virtual Machine Circuit
([#1268](#1268))
([f85ecd9](f85ecd9))
* Exposing nargo version via `NodeInfo`
([#2333](#2333))
([1c2669c](1c2669c)),
closes
[#2332](#2332)
* Migrate accounts to auth witness
([#2281](#2281))
([91152af](91152af)),
closes
[#2043](#2043)


### Bug Fixes

* Aztec-nr mirror url
([#2321](#2321))
([aaf7f67](aaf7f67))
* **build:** Fixed paths on s3 deployments
([#2335](#2335))
([38c7979](38c7979))


### Miscellaneous

* Do not format boxes with global format
([#2326](#2326))
([2fe845f](2fe845f))
* Remove native token
([#2280](#2280))
([4032d01](4032d01))
* Rename getAccounts to getRegisteredAccounts
([#2330](#2330))
([c7f3776](c7f3776))
</details>

<details><summary>barretenberg.js: 0.7.4</summary>

##
[0.7.4](barretenberg.js-v0.7.3...barretenberg.js-v0.7.4)
(2023-09-15)


### Miscellaneous

* **barretenberg.js:** Synchronize aztec-packages versions
</details>

<details><summary>barretenberg: 0.7.4</summary>

##
[0.7.4](barretenberg-v0.7.3...barretenberg-v0.7.4)
(2023-09-15)


### Features

* Elliptic Curve Virtual Machine Circuit
([#1268](#1268))
([f85ecd9](f85ecd9))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Maddiaa0 Maddiaa0 Maddiaa0 left review comments

@rahul-kothari rahul-kothari rahul-kothari approved these changes

@spalladino spalladino Awaiting requested review from spalladino

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace native_token and private_token in dapp_testing and update references in docs
3 participants
@LHerskind @rahul-kothari @Maddiaa0