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

chore: upgrade spl-token package #299

Merged
merged 16 commits into from
Mar 11, 2022
Merged

Conversation

thlorenz
Copy link
Contributor

@thlorenz thlorenz commented Mar 11, 2022

Description

Upgraded all packages except fixed-price-sale go the latest spl-token version.
This latest version introduced breaking API changes and the code using it was adapted.

The only package using actual methods of this package in production SDK code is token-vault,
all others are only using them as part of test setups or to reference the TOKEN_ID.

The new API exposes more instructions instead of transactions which is essential if we want to
expose an SDK that allows users to pick and choose which they need to include and build cost
efficient transactions.
Working on token-metadata triggered me to pull this update forward as I realized that I need
not only the instruction methods there, but also lots of other helper methods regarding mints
and tokens that were included with that API.

As outlined in the issue referenced below I did attempt to upgrade this for fixed-price-sale
as well but ran into too many issues with failing tests which point to timing issues which I
was unable to debug in the amount of time I wanted to spend on this update.

Core Update

The mpl core package also references spl-token in the code but was missing it as a dependency.
It is only using it for its TokenAccount wrapper.

This wrapper is only used in fixed-price in the validateMembershipToken which is exclusively used in its tests. Thus I expect this to be updated and the use become obsolete once fixed-price-sale also updates spl-token.
The wrapper is also used inside token-metadata, but a revamp of the SDK of that package is already in the works and thus this use will also become obsolete.

This core TokenAccount is based on an spl-core AccountInfo that as such doesn't exist in the new version (instead a similar class Account exists. Thus it is very difficult to easily update this use.

For those reasons I decided to remove TokenAccount from core with the intent to publish a new core version once those mentioned packages don't use this functionality anymore. Still that update should be a minor version bump.

NOTE: since core was published missing the spl-token dep in the package.json which didn't cause errors while all parent packages used the same version, I had to publish a patch @metaplex-foundation/mpl-core@0.0.5 and upgrade all packages to use this version in order to get CI to pass for this PR. This required to remove one obsolete query method in token-metadata since it required a now obsolete Account property to work. The removed method wasn't used anywhere in our packages.

As a side note mpl-core dep was missing from fixed-price-sale package as well which I fixed.

Metaplex Fix

The metaplex package was depending on token-vault types + consts that were removed in the meantime. This caused CI to fail and was fixed by adding both to metaplex proper (referring to consts exposed by the core package).

TypeScript Dependency Fix

  • most packages didn't directly reference the typescript dev dependency
  • this works for library only deps, but not for executables
  • thus we actually were using the tsc command installed globally on the respective machine instead of the one we meant it to use (local to the package or at least the repo)
  • I fixed this by adding this dependency where it is used

References

Testing

Where needed tests and supporting functions were updated to use the new API and I ensured that
they all pass after the change.

Next Steps

In order to have consistent dep versions across the repo the issue mentioned above should be
addressed ASAP.

@thlorenz thlorenz marked this pull request as draft March 11, 2022 00:45
@thlorenz thlorenz force-pushed the thlorenz/chore/upgrade-spl-token branch from a994011 to 3567b41 Compare March 11, 2022 03:48
- we had linked it so far to adapt API
- core is using this library only for `TokenAccount` wrapper
- TokenAccount wrapper is only used by fixed-price at this point and by
  token-metadata but only until the SDK is revamped (which is WIP)
@thlorenz thlorenz force-pushed the thlorenz/chore/upgrade-spl-token branch from 3567b41 to adc56c8 Compare March 11, 2022 04:03
@thlorenz thlorenz force-pushed the thlorenz/chore/upgrade-spl-token branch from aaf1c42 to d4ca326 Compare March 11, 2022 17:16
- executables need to be a dev dep for the package using it
- picking up from a workspace root does not work, instead it was picking
  up the `tsc` executable that was installed globally on the developer
  machine
@thlorenz thlorenz force-pushed the thlorenz/chore/upgrade-spl-token branch from 3b84211 to 12ece8c Compare March 11, 2022 17:47
@thlorenz thlorenz requested a review from lorisleiva March 11, 2022 18:02
@austbot austbot marked this pull request as ready for review March 11, 2022 18:05
@austbot austbot merged commit 72cf786 into master Mar 11, 2022
@thlorenz thlorenz deleted the thlorenz/chore/upgrade-spl-token branch March 11, 2022 23:04
thlorenz added a commit that referenced this pull request Mar 11, 2022
* master:
  Update Cargo.toml
  Take more bytes from slot for entropy (#300)
  chore: upgrade spl-token package (#299)
  Auction house docs (#296)
  Fix order in extra accounts comment to match (#298)
thlorenz added a commit that referenced this pull request Mar 11, 2022
* master:
  Update Cargo.toml
  Take more bytes from slot for entropy (#300)
  chore: upgrade spl-token package (#299)
  Auction house docs (#296)
  Fix order in extra accounts comment to match (#298)
  chore(CODEOWNERS): remove aaron from code owners (#292)
  Adds another account for collection update authority when authuser is not minting (#290)
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