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

Adds total supply view and updates cadence tests #139

Merged
merged 5 commits into from
Aug 9, 2023
Merged

Conversation

joshuahannan
Copy link
Member

@joshuahannan joshuahannan commented Jul 11, 2023

Description

  • Adds a view for the total supply of a token
  • Updates cadence, emulator, and sdk dependencies. All the changes to the tests and templates files don't need to be reviewed because they are just updates to dependencies.
  • Removes JS tests and add Cadence testing framework tests for the switchboard

For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels
  • Update the version in package.json when there is a change in the smart contracts

@joshuahannan joshuahannan added enhancement New feature or request SC-Eng Issues that we want to see surfaced in SC-Eng ZH Board labels Jul 11, 2023
@joshuahannan
Copy link
Member Author

@sisyphusSmiling @satyamakgec I'm having some issues with the JS test setup in this PR when I run the tests and when CI runs. Can one of you take a look? Thank you!

@sisyphusSmiling
Copy link
Contributor

sisyphusSmiling commented Jul 11, 2023

cc: @jribbink @franklywatson could use some js-friendly eyes

Not sure why the js tests are failing here. I'm getting some odd behavior - beforeEach() throws undefined and every other test fails with connection refused, each with differing ports:

HTTP Request Error:
          HTTP Request Error: An error occurred when interacting with the Access API.
          error=request to http://localhost:57873/v1/network/parameters failed, reason: connect ECONNREFUSED 127.0.0.1:57873
          hostname=http://localhost:57873
          path=/v1/network/parameters
          method=GET

I was able to get switchboard.test.js passing at some point and now they're all failing. For reference, here's my npm version output:

{
  test: '1.0.0',
  npm: '9.8.0',
  node: '16.16.0',
  v8: '9.4.146.24-node.21',
  uv: '1.43.0',
  zlib: '1.2.11',
  brotli: '1.0.9',
  ares: '1.18.1',
  modules: '93',
  nghttp2: '1.47.0',
  napi: '8',
  llhttp: '6.0.7',
  openssl: '1.1.1q+quic',
  cldr: '40.0',
  icu: '70.1',
  tz: '2021a3',
  unicode: '14.0',
  ngtcp2: '0.1.0-DEV',
  nghttp3: '0.1.0-DEV'
}

This is after updating to latest stable npm release from 8.11.0

@joshuahannan joshuahannan changed the title Adds total supply view and updates dependencies Adds total supply view and updates cadence tests Jul 31, 2023
@joshuahannan
Copy link
Member Author

@sisyphusSmiling and @satyamakgec I removed the JS tests and added some cadence tests for the switchboard, which was the only extra thing that wasn't already handled by the Go/Cadence tests, so this is ready for another review

Copy link
Contributor

@sisyphusSmiling sisyphusSmiling left a comment

Choose a reason for hiding this comment

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

LGTM. Glad to see we're getting more Cadence tests in the mix!

.borrow<&{MetadataViews.Resolver}>()
?? panic("Could not borrow a reference to the vault resolver")

let ftSupply = vaultRef.resolveView(Type<FungibleTokenMetadataViews.TotalSupply>())!
Copy link
Contributor

Choose a reason for hiding this comment

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

np: Maybe not in scope for this PR, but we could generalize this script if ExampleToken implemented ViewResolver. I think we should encourage NFT & FT contracts to implement that interface anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why would ExampleToken need to implement ViewResolver to make this generalizable? Isn't it already generalizable if you just provide the path as an argument instead of hardcoding? Agreed about making it implement ViewResolver though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, totally true. Could just add public path. I've just come across a couple of instances with HC scripts/txns where ViewResolver would make things easier, so I've been more inclined to spot opportunities to encourage the pattern. You're right though, either one works

@joshuahannan joshuahannan merged commit b723f1e into master Aug 9, 2023
1 check passed
@joshuahannan joshuahannan deleted the balance-views branch August 9, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request SC-Eng Issues that we want to see surfaced in SC-Eng ZH Board
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants