Add common @truffle/supplier #4224

gnidan · 2021-08-05T03:50:50Z

To build a supplier for a given software component (e.g. the solc CompilerSupplier), @truffle/supplier now exposes forDefinition({ determineStrategy, strategyConstructors }), which returns a createSupplier function as a result. createSupplier functions take a language-specific set of options, determine which of the defined strategies to use based on those options, then instantiate the determined strategy with those options.

This new package provides type-safety support by way of forDefinition's being generic to a particular Supplier Specification. Defining an explicit Supplier specification (and corresponding Specifications for each Strategy) will ensure that supplier.load(version?: string) and supplier.list() throws errors unless the type-checker can guarantee the strategy implements those methods, using type guards, e.g.:

const supplier = createCompilerSupplier(options);

// some strategies (e.g. Native) can't accept a specific version.
// without this check, TS will throw a type error
if (supplier.allowsLoadingSpecificVersion()) {
  const { solc } = await supplier.load("0.5.0");
  // ...
}

// some strategies don't allow you to list all versions
// another type check :)
if (supplier.allowsListingVersions()) {
  const { releases } = await supplier.list();
  // ...
}

Question: should this be moved to a new package (maybe @truffle/supplier)? There's nothing specific to compilers about it, really, and if we want to move common code for multiple languages here later, we might find ourselves adding dependencies to @truffle/compile-common that we don't want. (doing this)

gnidan · 2021-08-11T17:54:37Z

Question: should this be moved to a new package (maybe @truffle/supplier)? There's nothing specific to compilers about it, really, and if we want to move common code for multiple languages here later, we might find ourselves adding dependencies to @truffle/compile-common that we don't want.

(I opened #4248 in case the answer to this question is "yes")

- Create new method cache.getCachedFileNames() in order to remove coupling between VersionRange and compilerCachePath

As a potential stopgap: 1. Blindly replace all `noVersion` / `noWhatever` strings with analogous `NoVersionError` classes, putting them in new errors bucket module 2. For each such new error class, if the class is used by only one loading strategy, move the class to that module.

- Stop doing a separate `this.loadParserSolc()` inside `supplier.load()` - Remove `parserSolc` from the return value of `supplier.load()`, since the invoker of this method can just load a specific parser itself, and forcing this cleans up the CompilerSupplier interface / logic flow - Add the parserSolc selection logic to the only place in Truffle that uses the now-gone return value. (!! thus breaking @truffle/compile-solidity !!)

- Define new unified method for listing versions, which returns { latestRelease, prereleases, releases}, where `prereleases` and `releases` may both be AsyncIterableIterators - Align VersionRange with new interface for this method - Replace naïve `docker.getDockerTags()`, which only returns the first page of results from the Hub API: - Implement API page traversal as an AsyncIterableIterator of all tags individually. - Add retry logic to axios to perform exponential backoff to get around Docker Hub's 429 Too Many Requests responses - Use iter-tools to fork/filter/conform unified stream into `{ latestRelease, prereleases, releases }` - Add warning not to do `truffle compile --list=docker --all`

- Rename cache.fileIsCached() to cache.has() - Rename cache.addFileToCache() to cache.add() - Rename cache.resolveCache() to cache.resolve() - Rename cache.getCachedFileNames() to cache.list()

Rename `getSolcByUrlAndCache` to `getAndCacheSolcByUrl`

Make dedicated @truffle/supplier package

haltman-at

OK, further comments on this. I remain really wary of the whole thing due to the complexity of the types; I'm still not sure that this is necessary, and that you couldn't get most of the effect a simpler (if looser) way.

Also, reviewing these would have been a lot easier had I read from the bottom up! Each thing is defined in terms of things below it, and those components below it aren't easily-blackboxed abstractions, but rather preliminaries that need to be understood first for the whole thing to make sense.

packages/supplier/package.json

packages/supplier/src/strategy.ts

haltman-at · 2021-08-19T22:11:33Z

packages/supplier/src/supplier.ts

+  const strategyName = determineStrategy(options);
+  const Strategy = strategyConstructors[strategyName];
+
+  // @ts-ignore since we can't figure out N from inside


Using ts-ignore in ordinary code makes me really wary. Can't this just be a coercion instead?

I'm of the mind now that @ts-ignore is safer than coercions

I strongly disagree with this point of view. I do not think we should be using ts-ignore... basically at all. We used it in that one place in encoder only because the configuration inconsistency (where the same code had to compile with strictNullChecks both on and off) left us with basically no other choice.

In general, we should try to work within TS, not ignore it. Obviously coercions are not fully "working within TS", but they're closer. TS does for instance perform rudimentary checks on coercions to make you're not sure you're doing anything too stupid with them, for instance; it obviously doesn't do that with ts-ignore.

You have elsewhere, although not here, made arguments as to why coercion is unsafe. I do not see that any of these arguments apply here. Indeed, I'm not convinced those arguments apply much of anywhere, as you've never provided any concrete examples, and the ones you've vaguely described seem to me to rely on what I would consider to be other bad practices; I would say those are the problems, not the coercion.

If you want to convince me on this point you are going to need to provide much more in the way of argument.

I would strongly discourage the use of ts-ignore both here and everywhere.

packages/supplier/src/supplier.ts

haltman-at · 2021-08-19T22:15:41Z

packages/supplier/src/supplier.ts

+     * name.
+     */
+    strategies: {
+      [strategyName: string]: FreeStrategy.Specification;


Weren't you going to force results agreement here? I don't see anything here that forces that.

Unfortunately, there's not a good way to do this enforcement here; since this is the base specification type, there's not a good way to enforce the cross-linkage.

The enforcement happens elsewhere: forDefinition will fail to type-check if given an S whose strategies' results types do not agree with each other.

Oh, hm. Well, if you can't enforce it in the type system proper, I'd suggest at least adding a comment to indicate that that's how you should use it. (Comments: The poor programmer's type system! :P )

haltman-at · 2021-08-19T22:27:37Z

packages/supplier/src/supplier.ts

+ */
+export type Supplier<
+  S extends Supplier.Specification,
+  N extends Supplier.StrategyName<S> = Supplier.StrategyName<S>


What is the difference between

N extends Supplier.StrategyName<S> = Supplier.StrategyName<S>

and just

N extends Supplier.StrategyName<S>

?

The former allows the parameter to be omitted, so you can do Supplier<S> instead of Supplier<S, N>

Oh! It's a default value. I see, I didn't get that. Perhaps a comment indicating that would be useful?

I revised the docstring, hopefully it's clear enough from that?

Oh yeah, revising the docstring is a better idea. What I was confused about was the syntax, so I was thinking of just adding a comment to clear up the syntax, but revising the docstring is actually probably the better idea.

Unfortunately the new material that you added to the docstring is not entirely clear. When you write "When N is omitted, Supplier<S> represents a supplier that uses any specified strategy"... "any specified strategy" is a terribly unclear phrase. No strategy was specified! I eventually figured out that you meant "any of the strategies appearing in the specification S".

haltman-at · 2021-08-19T22:28:59Z

packages/supplier/src/supplier.ts

+export type Supplier<
+  S extends Supplier.Specification,
+  N extends Supplier.StrategyName<S> = Supplier.StrategyName<S>
+> = Supplier.StrategyName<S> extends N


I do not understand what this ternary is doing. Or rather, I think understand what it's doing, but if so I don't understand why.

If N extends T (enforced by the generic param) and T extends N, then N equals T. So this distinguishes supplier types for any strategy name vs. a specific strategy name.

I'll add a comment for this.

I still don't understand this, sorry. Admittedly you haven't written your comment yet. But what you've said only explains the condition of the ternary. That doesn't explain why you would use BaseStrategy in the one case and FreeStrategy in the other. The thing I don't understand about the ternary is that the effect of the condition seems, as best I can tell, to be unrelated to the condition.

I added the comment - but maybe not enough explanation of the "why"... I'll take a look and see what I can do about that

packages/supplier/src/supplier.ts

haltman-at · 2021-08-19T22:40:21Z

packages/supplier/src/supplier.ts

+
+  export namespace Strategies {
+    /**
+     * Type-level specification of all strategies for a specified supplier


Specifically, this is the mapping from names to strategies, yes?

Mapping from names to strategy specifications, yep.

Sorry, I should be more explicit. I think the comment should state that.

packages/supplier/README.md

- Remove unused colors - Move ts-mixer to devDependencies

- Ensure generic parameters are documented - Extend optionality of N through a few types where appropriate - Define a standalone `Supplier.Strategy<S, N>` type

gnidan · 2021-08-20T21:23:15Z

Also, reviewing these would have been a lot easier had I read from the bottom up! Each thing is defined in terms of things below it, and those components below it aren't easily-blackboxed abstractions, but rather preliminaries that need to be understood first for the whole thing to make sense.

If you recall our discussion about this, it seems that the question of this directionality hinges on the open question of "which exports are truly external?" My stance is that the exports towards the bottom of the file are inherently more internal, and thus should not be at the top. Do you disagree and think that these types are intrinsically an externally-facing concern?

OK, further comments on this. I remain really wary of the whole thing due to the complexity of the types; I'm still not sure that this is necessary, and that you couldn't get most of the effect a simpler (if looser) way.

So, the question is: is the additional type safety here worth the cognitive load? My thinking (obv.) is yes: I want the compiler to tell people who are building supplier strategies that they must know upfront (e.g.) if their strategy allows listing versions. I want new suppliers and new strategies for existing suppliers to require as little duplication as possible, while still making sure that all resultant createSupplier(options) functions are well-typed. I want to prevent mismatched return types for different strategies' load() methods. In my experience, these sorts guarantees are tremendously helpful.

I posit, thus, that this is a good trade-off: the cognitive load when implementing a new supplier is that you must figure out how to write a specification type; in return, you won't have to worry about figuring out what type something is anywhere else; the compiler will tell you when you mess up, and in the future, changes to that supplier won't run the risk of being incomplete.

(In case I'm not getting my full argument across: by counterexample, think of all the places in the code that one needs to touch just to do something as simple as add a field to @truffle/contract-schema. That's the sort of thing I'm trying to avoid with these specification types.)

haltman-at · 2021-08-21T00:33:58Z

If you recall our discussion about this, it seems that the question of this directionality hinges on the open question of "which exports are truly external?" My stance is that the exports towards the bottom of the file are inherently more internal, and thus should not be at the top. Do you disagree and think that these types are intrinsically an externally-facing concern?

The idea of some things as internal and some things as external relies on the idea that interface can be separated from implementation, that the user can focus on certain things and ignore their internals. Like, if you define a function, the user shouldn't need to look at what other functions that function calls to understand it; they shouldn't need to read the body at all. You black-box the implementation and just focus on the interface.

The problem is, with types, there is very little distinction between a type's "interface" and it's "implementation". Frankly I have little idea how I'd separate the two. To understand a type, you generally need to have some understanding of what each of its components means. This is how you come to understand a type -- by reading each of the components and the comments on them. The whole thing is both interface and implementation; there's no separate body you can ignore!

Now often you don't need to understand all the components immediately and fully, so you can sensibly work top-down... but this relies on the components making some sort of a priori intuitive sense, so that you can have some reasonable provisional idea about what they are. That doesn't apply here, because none of these types are intuitive. My reaction on reading these isn't "oh OK I guess that's a blah, I can check the details of it later", it's, "Huh? what the hell is a blah? Guess I'd better stop what I'm reading and go jump to that and see". And now I'm trying to figure out two confusing types.

Basically I don't think much of any of this is internal, because, well... it's types. I don't know how one could be confident that they could black-box much of any of this.

...OK I got kind of pulled away before replying to the rest of this, I think to make sure this at least goes up I'm going to just hit post and can reply to the rest after in a separate comment...

haltman-at · 2021-08-21T00:49:04Z

So, the question is: is the additional type safety here worth the cognitive load? My thinking (obv.) is yes: I want the compiler to tell people who are building supplier strategies that they must know upfront (e.g.) if their strategy allows listing versions. I want new suppliers and new strategies for existing suppliers to require as little duplication as possible, while still making sure that all resultant createSupplier(options) functions are well-typed. I want to prevent mismatched return types for different strategies' load() methods. In my experience, these sorts guarantees are tremendously helpful.

Again, I think you could still get most of the type safety without such complication.

But I think there's another problem here: What new suppliers and new strategies? How many of these are actually going to get written?

My understanding was that the point of this was to allow the writing of a Vyper CompilerSupplier. Note, that's specifically a second complier supplier; it doesn't require a more general "supplier" notion. And, um, do we expect to be writing any more suppliers than that...?

I guess what I'm realizing now is that part of my objection is that this is a lot of abstraction that is hardly ever going to get used. You're trying to abstract things that we simply have no plan for. Better to just make an abstraction that covers the things we actually plan to do.

Actually, this makes me realize there's some bits of the design I hadn't properly processed and criticized before, but I'll post that in a separate comment.

I posit, thus, that this is a good trade-off: the cognitive load when implementing a new supplier is that you must figure out how to write a specification type; in return, you won't have to worry about figuring out what type something is anywhere else; the compiler will tell you when you mess up, and in the future, changes to that supplier won't run the risk of being incomplete.

I don't think "you won't have to worry about figuring out what type something is anywhere else" is a good goal. Like, you should always have types on your functions.

(In case I'm not getting my full argument across: by counterexample, think of all the places in the code that one needs to touch just to do something as simple as add a field to @truffle/contract-schema. That's the sort of thing I'm trying to avoid with these specification types.)

What makes you think that anything like that situation would arise without this mechanism?

haltman-at · 2021-08-21T00:57:32Z

OK, so, here's something I only just realized; Why do strategies have names?

In the existing way, strategies don't have names. This whole strategy-name thing is invented here.

Like, imagine we stuck closer to the original code. Then instead of having determineStrategy which returns a name, and having strategies indexed by those names, we would instead simply have a list of strategies, and each strategy instead of having a name would have an isApplicable (or something) predicate, and determineStrategy, rather than being something user-supplied, would just consist of checking the list of strategies in order to find the first one that's applicable.

The whole strategy-name thing seems like a needless complication. What if that were cut out? (Although, perhaps it's impossible to do all the type things you want to do if that were cut out?)

eggplantzzz · 2021-08-25T17:07:39Z

packages/supplier/src/strategy.ts

+     *
+     * When specifying a strategy, this must be `true` or `false` explicitly.
+     */
+    allowsLoadingSpecificVersion: boolean;


Are these supposed to be methods? The mixin specifies a function that returns the boolean.

gnidan · 2021-11-12T01:50:48Z

Closing this for cleanup. If we want to continue on this front, I think it might be worth starting from tabula rasa

gnidan added the new-feature: @truffle/compile-common label Aug 5, 2021

gnidan mentioned this pull request Aug 5, 2021

Use @truffle/supplier to rewrite @truffle/compile-solidity's CompilerSupplier #4225

Closed

gnidan force-pushed the supplier/common branch from 9f94b55 to 4252a55 Compare August 5, 2021 03:56

gnidan changed the base branch from supplier/refactor-js to develop August 5, 2021 03:57

gnidan force-pushed the supplier/common branch from 4252a55 to 1d795f5 Compare August 5, 2021 04:01

gnidan marked this pull request as ready for review August 5, 2021 04:13

gnidan requested review from eggplantzzz, cds-amal, davidmurdoch and haltman-at August 5, 2021 22:07

gnidan mentioned this pull request Aug 11, 2021

Make dedicated @truffle/supplier package #4248

Merged

gnidan mentioned this pull request Aug 12, 2021

Refactor CompilerSupplier #4222

Merged

g. nicholas d'andrea added 17 commits August 12, 2021 21:07

Add supplier/* branches to GHA

d52234e

Move normalizeSolcVersion out of VersionRange

ae27ad6

Separate LoadingStrategy cache into own class

e8bd833

- Create new method cache.getCachedFileNames() in order to remove coupling between VersionRange and compilerCachePath

Extract listener cleanup into function

7c4afa5

Remove supplier.downloadAndCacheSolc

322a3c4

Remove LoadingStrategy base class

cd734fb

Move public methods above private ones

aa1318a

Rename Cache methods

317a12d

- Rename cache.fileIsCached() to cache.has() - Rename cache.addFileToCache() to cache.add() - Rename cache.resolveCache() to cache.resolve() - Rename cache.getCachedFileNames() to cache.list()

Add suggested comments

615e5a7

Remove extra method in Local strategy

cb8decd

Rethrow caught errors instead of making new ones

2a51b86

Remove weird JSON.stringify rename

1d1b6c6

Clarify method name for getting+caching

e1752da

Rename `getSolcByUrlAndCache` to `getAndCacheSolcByUrl`

Remove unused getSolcByCommit()

1411d3b

gnidan removed the new-feature: @truffle/compile-common label Aug 16, 2021

gnidan changed the title ~~Add CompilerSupplier support to @truffle/compile-common~~ Add common @truffle/supplier Aug 16, 2021

g. nicholas d'andrea and others added 4 commits August 17, 2021 20:47

Add CompilerSupplier support in compile-common

10e4820

Make dedicated @truffle/supplier package

a59cef2

Merge pull request #4248 from trufflesuite/supplier/package

08e19b6

Make dedicated @truffle/supplier package

Ensure all types get docstrings

5499b2e

gnidan force-pushed the supplier/common branch from 375a5e1 to 5499b2e Compare August 18, 2021 00:50

gnidan requested a review from haltman-at August 18, 2021 00:51

Extract load/list interfaces

bdd85cc

haltman-at reviewed Aug 19, 2021

View reviewed changes

g. nicholas d'andrea added 5 commits August 20, 2021 16:19

Allow optional N in Supplier.Options<S, N>

25e7457

Fix dependencies

78e36ea

- Remove unused colors - Move ts-mixer to devDependencies

Explain Supplier<S, N> generic params

379ffee

Clean up generics a bit

bdb8ec0

- Ensure generic parameters are documented - Extend optionality of N through a few types where appropriate - Define a standalone `Supplier.Strategy<S, N>` type

Add mixin-free usage example to README

710afa7

gnidan requested a review from haltman-at August 20, 2021 21:23

eggplantzzz reviewed Aug 25, 2021

View reviewed changes

gnidan force-pushed the supplier/main branch 5 times, most recently from 73511bb to f5adb64 Compare November 12, 2021 01:50

gnidan closed this Nov 12, 2021

gnidan deleted the supplier/common branch November 12, 2021 01:51

vutting4221 mentioned this pull request Oct 18, 2024

[Snyk] Fix for 1 vulnerabilities vutting4221/truffle#204

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add common @truffle/supplier #4224

Add common @truffle/supplier #4224

gnidan commented Aug 5, 2021 •

edited

Loading

gnidan commented Aug 11, 2021

haltman-at left a comment

haltman-at Aug 19, 2021

gnidan Aug 20, 2021

haltman-at Aug 20, 2021

haltman-at Aug 19, 2021

gnidan Aug 20, 2021

haltman-at Aug 20, 2021 •

edited

Loading

haltman-at Aug 19, 2021

gnidan Aug 20, 2021

haltman-at Aug 20, 2021

gnidan Aug 20, 2021

haltman-at Aug 21, 2021 •

edited

Loading

haltman-at Aug 19, 2021

gnidan Aug 20, 2021

haltman-at Aug 20, 2021

gnidan Aug 20, 2021 •

edited

Loading

haltman-at Aug 19, 2021

gnidan Aug 20, 2021

haltman-at Aug 20, 2021

gnidan commented Aug 20, 2021

haltman-at commented Aug 21, 2021

haltman-at commented Aug 21, 2021

haltman-at commented Aug 21, 2021

eggplantzzz Aug 25, 2021

gnidan commented Nov 12, 2021

Add common @truffle/supplier #4224

Add common @truffle/supplier #4224

Conversation

gnidan commented Aug 5, 2021 • edited Loading

gnidan commented Aug 11, 2021

haltman-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haltman-at Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haltman-at Aug 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnidan Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnidan commented Aug 20, 2021

haltman-at commented Aug 21, 2021

haltman-at commented Aug 21, 2021

haltman-at commented Aug 21, 2021

Choose a reason for hiding this comment

gnidan commented Nov 12, 2021

gnidan commented Aug 5, 2021 •

edited

Loading

haltman-at Aug 20, 2021 •

edited

Loading

haltman-at Aug 21, 2021 •

edited

Loading

gnidan Aug 20, 2021 •

edited

Loading