sdk/vm should perform precompiling and basic validation on MsgAddPkg
#1661
Comments
|
@petar-dambovaliev FYI, I made an implementation of this in #1695 which leverages the existing go type checker |
Then we don't have to do anything on this issue. |
thehowl
added a commit
that referenced
this issue
May 13, 2024
…key (#1702) Split from #1695 for ease of reviewing. Merge order: 1. #1700 2. #1702 (this one!) 3. #1695 \ #1730 This PR removes `TranspileAndCheckMempkg` in favour of performing the type checking it was supposed to do using `go/types` with a custom importer. This importer works together with Gno's `Store`, and can as such be used to type check Gno packages without ever writing a single file to disk. It is important to note that by "Go type check" I mean a variety of compile-time checks the Go compiler performs; in fact, this is much more powerful than running "gofmt" as we are currently doing. Additionally, it adds a new flag to gnokey, `-simulate`, to control transaction simulation before committing a transaction. See [this issue comment](#1702 (comment)) Resolves #1661. ## Reviewing notes - transpiler.TranspileAndCheckMempkg has been removed from the gnokey client and gnoclient, in favour of having this step be performed on the vm keeper. This paves the way for clients to not have to include the entire GnoVM, which I call a win. - Stdlib io had a precompiling error due to an unused variable (`remaining`); I updated it to the latest code on Go's standard libraries. - `Store` changes - `Store` has been changed to have its `getPackage` method work by detecting import cycles, without risking race conditions (the current implementation is not thread-safe). This is done by creating a new store, `importerStore`, which contains the previously imported paths in the current chain. Cyclic imports are still (correctly) detected in the tests. - `GetMemPackage` has been changed to return nil when a package cannot be found. This matches its behaviour with `GetMemFile`, which already did this when the file does not exist. - `GetMemPackage`, if a package is not found in the store, now attempts retrieving it using Store.GetPackage first. The underlying reason is that the Gno importer for the type checker needs to access the source of the standard libraries; however, these are never in any transaction and are not executed "per se" when the blockchain start. As a consequence, they may not exist within the Store; as a solution, when using GetMemPackage, we ensure that a package does not exist by checking if GetPackage does not retrieve it through getMemPackage and save it.
github-project-automation
bot
moved this from In Progress
to Done
in 🧙♂️gno.land core team
DIGIX666
pushed a commit
to kazai777/gno
that referenced
this issue
May 15, 2024
…key (gnolang#1702) Split from gnolang#1695 for ease of reviewing. Merge order: 1. gnolang#1700 2. gnolang#1702 (this one!) 3. gnolang#1695 \ gnolang#1730 This PR removes `TranspileAndCheckMempkg` in favour of performing the type checking it was supposed to do using `go/types` with a custom importer. This importer works together with Gno's `Store`, and can as such be used to type check Gno packages without ever writing a single file to disk. It is important to note that by "Go type check" I mean a variety of compile-time checks the Go compiler performs; in fact, this is much more powerful than running "gofmt" as we are currently doing. Additionally, it adds a new flag to gnokey, `-simulate`, to control transaction simulation before committing a transaction. See [this issue comment](gnolang#1702 (comment)) Resolves gnolang#1661. ## Reviewing notes - transpiler.TranspileAndCheckMempkg has been removed from the gnokey client and gnoclient, in favour of having this step be performed on the vm keeper. This paves the way for clients to not have to include the entire GnoVM, which I call a win. - Stdlib io had a precompiling error due to an unused variable (`remaining`); I updated it to the latest code on Go's standard libraries. - `Store` changes - `Store` has been changed to have its `getPackage` method work by detecting import cycles, without risking race conditions (the current implementation is not thread-safe). This is done by creating a new store, `importerStore`, which contains the previously imported paths in the current chain. Cyclic imports are still (correctly) detected in the tests. - `GetMemPackage` has been changed to return nil when a package cannot be found. This matches its behaviour with `GetMemFile`, which already did this when the file does not exist. - `GetMemPackage`, if a package is not found in the store, now attempts retrieving it using Store.GetPackage first. The underlying reason is that the Gno importer for the type checker needs to access the source of the standard libraries; however, these are never in any transaction and are not executed "per se" when the blockchain start. As a consequence, they may not exist within the Store; as a solution, when using GetMemPackage, we ensure that a package does not exist by checking if GetPackage does not retrieve it through getMemPackage and save it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Like it is being done client-side, the precompile check should be performed also server-side. This ensures code is correct for anything that we don't catch right now, including #1086.
The text was updated successfully, but these errors were encountered: