Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Decode requests in dashboard #5621

Merged
merged 90 commits into from
Nov 18, 2022
Merged

Decode requests in dashboard #5621

merged 90 commits into from
Nov 18, 2022

Conversation

cliffoo
Copy link
Contributor

@cliffoo cliffoo commented Oct 16, 2022

Addressed

#4897

Supported methods

eth_sendTransaction, personal_sign, eth_signTypedData_v3, eth_signTypedData_v4

Unsupported methods

eth_decrypt: MM says it's deprecated.
eth_signTypedData: I just couldn't get a valid request to send.
eth_signTypedData_v1: This is supposed to be the same as eth_signTypedData, but MM seems to think it's not a valid method.

Screenshots

screenshot

screenshot

screenshot

screenshot

Idea

diagram

Other things

  • Using the decoder means various polyfills are required, which the latest react-scripts cannot provide. The CRA setup is replaced with webpack + babel + eslint, with minimal loaders and plugins. I also took this opportunity to freshen up the dev server scripts.
  • Removed cpy-cli devDep. This is no longer needed as the build command targets the dist folder directly.

Thanks

@gnidan, @haltman-at for answering my questions! This PR was helpful also.

@haltman-at
Copy link
Contributor

Using the decoder means web3js is now a transitive dependency, which the latest react-scripts cannot handle.

Oh hm, how'd that happen? We took web3 out of the decoder proper a while ago, although it's still in there as a devdep for testing. We are still using web3-utils. Wondering where the problem is coming from?

@cliffoo
Copy link
Contributor Author

cliffoo commented Oct 31, 2022

I misspoke... It's not web3, but other deps (including web3-utils) that require polyfills when webpacking decoder. I've updated my comments.

Copy link
Contributor

@benjamincburns benjamincburns left a comment

Choose a reason for hiding this comment

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

Looks good overall - I think there's just one change that I'd really like to see before approving (the message type stuff), and while I'd also like to swap babel-loader for ts-loader, I think I could be convinced to leave it as-is.

Comment on lines 30 to 38
export type WorkflowCompileResultMessageType = "workflow-compile-result";
export const workflowCompileResultMessageType = "workflow-compile-result";
/**
* Message to inform subscribers of new WorkflowCompileResult
*/
export interface WorkflowCompileResultMessage extends Message {
type: WorkflowCompileResultMessageType;
payload: WorkflowCompileResult;
}
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 incohesive to define such a specific message type here.

The message bus common package is meant to house logic and types that are common between the message bus service and the message bus client. In general I think it makes sense to define a generic message envelope in here, but I think defining the structure of messages that are specific to some other package (compile-common in this case) is probably a code smell.

The easy test for why it's problematic is that you need to take a dependency on compile-common here. Perhaps this could be avoided by defining a more generic and extensible message envelope type here that you could use to embed this same information? For example, for messages to do with RPC calls we just have the DashboardProviderMessage type. We don't have one message type per type of RPC call, however.

Perhaps there's a more generic CLIEventMessageType structure that would work better here?

If you aren't familiar with how other message busses work, it might be worth having a look at the message envelope types used by MQTT and AMQP.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I think it makes sense to define a generic message envelope in here, but I think defining the structure of messages that are specific to some other package (compile-common in this case) is probably a code smell.

Put differently - there's no reason why any of the core message bus service logic must know anything at all about WorkflowCompileResult in order do its job effectively. It should be able to treat that as opaque data that it's just shuffling around.

Copy link
Contributor Author

@cliffoo cliffoo Nov 15, 2022

Choose a reason for hiding this comment

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

Thanks Ben. Initially I was uncertain about having an envelope message type that represents any CLI event data, mostly because the payload must necessarily be typed any (I've typed payload data unknown by default). But I figured that I can just type it WorkflowCompileResult in the frontend, which admittedly isn't as nice as the given type guarantees ensured by isWorkflowCompileResultMessage, but this is a worthwhile tradeoff compared to taking compile-common as a dependency and making more specific message types in the future as dashboard grows, which does smell.

I've made updates to use CliEventMessage and got rid of WorkflowCompileResultMessage.

packages/dashboard/dev-server/all.ts Show resolved Hide resolved
packages/dashboard/dev-server/dashboard.ts Show resolved Hide resolved
packages/dashboard/lib/DashboardServer.ts Show resolved Hide resolved
packages/dashboard/package.json Outdated Show resolved Hide resolved
packages/dashboard/tsconfig.json Outdated Show resolved Hide resolved
packages/dashboard/webpack.config.ts Outdated Show resolved Hide resolved
packages/dashboard/webpack.config.ts Outdated Show resolved Hide resolved
packages/events/defaultSubscribers/dashboard.js Outdated Show resolved Hide resolved
Copy link
Contributor

@benjamincburns benjamincburns left a comment

Choose a reason for hiding this comment

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

Approved, but with a few open questions. Please read through my new comments prior to merging, as you may want to make further updates first.

Also when you do opt to merge, I'd very much be a fan of selectively or even completely squashing some of the 79 commits in this PR. I know some people aren't fans of that practice, but I have to wonder if all 79 commits build successfully. If you're confident that they do, feel free to ignore me. However if you think they might not - please bear in mind that commits that don't build properly break git bisect, which is a very powerful debugging tool.

Comment on lines 12 to 24
export type CliEventMessageType = "cli-event";
export const cliEventMessageType = "cli-event";
/**
* Message to inform subscribers of Truffle CLI event data.
* The message payload label helps identify the type of event data.
*/
export interface CliEventMessage extends Message {
type: CliEventMessageType;
payload: {
label: string;
data: any;
};
}
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 this is fine as-is for now, but you might want to consider making the type of data a generic just to force the consumer/producer to type things properly at the ingress/egress points.

Otherwise in the future it'd be great if the payload portion of this type into a new @truffle/event-messages package, or some other existing place like @truffle/events.

If you do that, then you can make use of discriminated unions to avoid the any, and more importantly to make it really easy to ensure that you've handled all potential types of payload in the inevitable switch/case statement that you'll have to write in the frontend where incoming events are handled.

Personally in other projects I've gone with the (admittedly annoying) approach of creating new modules to contain pure types like these, as otherwise you have to take a dependency on a much "fatter" package (like @truffle/events) from frontend code.

All of that said, if we go that route, I'd much prefer to see that done in a future PR, as I think this is fine for now and it'll unblock my work on TypeScript support.

Copy link
Contributor

Choose a reason for hiding this comment

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

As an aside, this is more reason to switch over to aedes/MQTT, as if we were using that we'd have been confronted with the need to split these message types into packages that are specific to their function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @benjamincburns. I'll definitely dig into other message brokers (especially MQTT) in the near future as dashboard's messaging requirements grow. Though I do wonder if adding another dashboard package is better than squishing the types into @truffle/dashboard-message-bus-common

The message bus common package is meant to house logic and types that are common between the message bus service and the message bus client

Something like @truffle/event-messages would serve the purpose of providing types to all things message bus, which falls into the domain of the common package?

Ah and about using generics here: I considered it but since the entire message lifecycle for CliEventMessage isn't useful at the moment, (this may very well change; right now it's just abandoned after publish, so only message.payload.data is useful), I opted to simply pass the payload data to a function (handleWorkflowCompileResult) whose params are typed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like @truffle/event-messages would serve the purpose of providing types to all things message bus, which falls into the domain of the common package?

My thinking in the case of a hyptothetical @truffle/event-messages was that the only consumers of that package would be event message producers or consumers. @truffle/dashboard-message-bus-common is really only meant to contain code that's common to both the message bus client and the message bus service itself, and my thought was that it should only contain logic and/or types that is strictly required for the proper functioning of both. In my ideal world the only direct consumers of @truffle/dashboard-message-bus-common would be @truffle/dashboard-message-bus and @truffle/dashboard-message-bus-client.

Honestly it's probably a mistake on my part to have created the common package without moving the message payload types elsewhere.

Copy link
Contributor

@benjamincburns benjamincburns Nov 16, 2022

Choose a reason for hiding this comment

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

Ah and about using generics here: I considered it but since the entire message lifecycle for CliEventMessage isn't useful at the moment, (this may very well change; right now it's just abandoned after publish, so only message.payload.data is useful), I opted to simply pass the payload data to a function (handleWorkflowCompileResult) whose params are typed.

You may want to consider changing the type of data from any to unknown, as that will force an explicit cast. I don't feel strongly about that preference, however. As a pattern it makes the code that consumes the unknown-typed field more explicit and readable, but in this specific case I don't think that it's likely to add a ton of extra value over any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You may want to consider changing the type of data from any to unknown

Done!

packages/dashboard-message-bus-common/package.json Outdated Show resolved Hide resolved
packages/dashboard/src/utils/dash.ts Show resolved Hide resolved
packages/dashboard/webpack.config.ts Show resolved Hide resolved
packages/dashboard/webpack.config.ts Outdated Show resolved Hide resolved
// - Batch inserting.
// - Mirroring some kind of db state.
for (const compilation of compilations) {
const hash = sha1(compilation);
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we know whether we've chosen the correct version of the workflow compile result for the transaction in question? I see that everything is keyed on this sha1 hash of the entire WorkflowCompileResult, but that won't allow for matching to a specific version of a given contract, will it?

A better approach might be to store off this hash, but also an index of compliations that's keyed by the hash of the deployed contract bytecode. That would let you verify if you're decoding against the correct/deployed version by hashing the result of eth_getCode and checking for it in the database.

This would also allow for less arbitrary cache invalidation strategies, as you could treat the database as an LRU cache and only keep around the most recently used N versions of each contract, where N is ideally some user-configurable value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea so the db essentially maps compilation hash to compilation + timestamp.

I see two things here:

What if workflow-compile changes?

At the moment it's only dependent on the shape of Compilation, so it that changes, it's probably time to migrate db.

The current schema means we can't match, hence decode specific version of a given contract?

So the reason for storing compilations is that one compilation is the unit; you cannot break it into e.g. sources, contracts, etc. and reconstruct it at a later time. That being said, all compilations are fed to one decoder, which means if you write contract A, compile, modify A to be A', compile, the decoder will use compilation for A' to decode.

One caveat as noted by this comment is that in some cases the bytecodes are the same despite having different contracts (like a change in param name), in which case with this week's release the decoder will prefer the "latest" compilation.

@cliffoo
Copy link
Contributor Author

cliffoo commented Nov 15, 2022

Thanks @benjamincburns! I'm fairly confident that each commit builds successfully, but I'm totally fine with squashing also.

Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

Some comments/questions for you. Thanks for shepherding this with so much care!

@@ -39,42 +37,62 @@
"@mantine/hooks": "^5.0.0",
"@mantine/notifications": "^5.0.0",
"@mantine/prism": "^5.0.0",
"@truffle/codec": "^0.14.8",
"@truffle/compile-common": "^0.9.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a devDependency?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I guess you might be using it for the shims.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the shims

Nah just using the types for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah so then maybe it can be a devDep!

Copy link
Contributor

Choose a reason for hiding this comment

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

@gnidan I thought your opinion on these types (which are exposed through TS magic) was that they should really be regular dependencies

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, I assumed that dashboard didn't expose these... are they exposed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope it's all lost at compile time. It's used here and 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.

Updated to be devDep!

Copy link
Member

Choose a reason for hiding this comment

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

I thought the reason to use deps was to make sure types don't break when the project is added as a dependency of another project via npm/yarn.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the reason to use deps was to make sure types don't break when the project is added as a dependency of another project via npm/yarn.

Not sure what you mean here, but we only need a regular dep if the package runs the dependency's code (trivial case) or if the package exports something that references one of the dependency's types, like as argument or return value for an exported function (case we're discussing)

packages/dashboard/src/utils/constants.ts Show resolved Hide resolved
packages/dashboard/src/utils/dash.ts Show resolved Hide resolved
async function ({ result }) {
try {
const publishLifecycle = await this.messageBusClient.publish({
type: "cli-event",
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the value of a generic cli-event, vs. making a dedicated workflow-compile-result message type? Do we really want to shoehorn a bunch of different events like this? I know Ben was talking about making things generic to avoid the use of any, but we could also... just make message types for message types we expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm torn, Ben's most convincing comment imo is this one. Maybe we can reconsider this one when dashboard starts receiving CLI events of more types?

As an aside, I just thought about the plugin system and it will probably require a generic message type similar to this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, my biggest (albeit not my only) concern about the specific message type was that it was causing us to take a dependency on workflow-compile from dashboard-message-bus-common.

My thought there is that over time as the scope of the dashboard grows, it's likely going to require data from a bunch of different packages. My fear is that if we establish a pattern of coupling generic/core/common packages to specific/functional packages we're likely going to run into a bunch of obnoxious problems (e.g. circular dependencies) down the line. Admittedly in this case the logic wasn't tightly coupled, but in my view that's more reason to avoid taking the direct dependency.

As a secondary concern, I think it's also usually a bad sign for the maintainability of an architecture when "transport" systems need to be made aware of the detailed structure of the data that it's passing. Ideally we'd have a single envelope type for all messages, there would be some topic key in that type, and the bus would just route messages to relevant subscribers based on that topic key. The payload field of that message envelope would also ideally be typed unknown so that we'd get compiler errors if anything w/i the message bus logic tried to interact with it directly.

Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

Couple more comments. I'm getting excited :D

decoderCompilationHashes.add(row.dataHash);
});

const decoder = await forProject({
Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies if I missed it, but is this now using the new interface to add compilations to the decoder?

(If it's not already using this interface, it can probably be a separate task if you want, but should at least open an issue to do that switch.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I'll leave that as a follow-up, we still re-init the decoder on new compilations.
I'll open an issue once this is merged, thanks for the reminder!

Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

This one's good for me if it's good for others :D

@gnidan
Copy link
Contributor

gnidan commented Nov 18, 2022

Oh shoot... one more thing, although I won't say it should be blocking:

The "could not decode" error suggests running truffle compile --all. This doesn't make sense for Hardhat users using Truffle Dashboard.

I guess medium term we should only suggest the Truffle command when we know it's a Truffle project... and slightly mediumer term we could just use @truffle/from-hardhat for actual decoding :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants