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

lib/runtime: implement ext_local_storage_set and ext_local_storage_get #1101

Merged
merged 7 commits into from
Sep 22, 2020

Conversation

edwardmack
Copy link
Contributor

@edwardmack edwardmack commented Sep 16, 2020

Changes

  • Created BasicStorage interface and NodeStorage struct to use for runtime offchain worker storage.
  • Implemented ext_local_storage_set and ext_local_storage_get functions

Tests

go test ./lib/runtime/...

Checklist

  • I have read CODE_OF_CONDUCT and CONTRIBUTING
  • I have provided as much information as possible and necessary
  • I have reviewed my own pull request before requesting a review
  • All integration tests and required coverage checks are passing

Issues

@edwardmack edwardmack self-assigned this Sep 17, 2020
@edwardmack edwardmack requested a review from noot September 17, 2020 15:38
dot/services.go Outdated Show resolved Hide resolved
dot/services.go Outdated Show resolved Hide resolved
lib/runtime/imports_old.go Outdated Show resolved Hide resolved
// You should have received a copy of the GNU Lesser General Public License
// along with the gossamer library. If not, see <http://www.gnu.org/licenses/>.

package runtime
Copy link
Contributor

Choose a reason for hiding this comment

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

it might make more sense to have this in the dot/state package, since that's where all the database related functionality is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when I move this to dot/state I get an import cycle, any suggestions?

package github.com/ChainSafe/gossamer/lib/runtime
	imports github.com/ChainSafe/gossamer/dot/state
	imports github.com/ChainSafe/gossamer/lib/genesis
	imports github.com/ChainSafe/gossamer/lib/runtime: import cycle not allowed

Comment on lines 40 to 57
// NewNodeStorageDB instantiates badgerDB instance for storing runtime persistent data
func NewNodeStorageDB(db chaindb.Database) *NodeStorageDB {
return &NodeStorageDB{
db,
}
}

// Put appends `storage` to the key and sets the key-value pair in the db
func (storageDB *NodeStorageDB) Put(key, value []byte) error {
key = append(storagePrefix, key...)
return storageDB.db.Put(key, value)
}

// Get appends `storage` to the key and retrieves the value from the db
func (storageDB *NodeStorageDB) Get(key []byte) ([]byte, error) {
key = append(storagePrefix, key...)
return storageDB.db.Get(key)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice to use table to add the prefixes instead of doing it manually, see #1091

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is much better.

Copy link
Contributor

@noot noot left a comment

Choose a reason for hiding this comment

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

overall looks good! please clean up todos and move database functionality to dot/state

@edwardmack edwardmack requested a review from noot September 18, 2020 20:03
@edwardmack
Copy link
Contributor Author

This is ready for a re-review.

@@ -29,21 +29,35 @@ import (
var memory, memErr = wasm.NewMemory(17, 0)
var logger = log.New("pkg", "runtime")

// NodeStorageTypePersistent flog to identify offchain storage as persistent (db)
Copy link
Contributor

Choose a reason for hiding this comment

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

*flag

Copy link
Contributor

@noot noot left a comment

Choose a reason for hiding this comment

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

nice work :)

@edwardmack edwardmack merged commit 6d70dbe into development Sep 22, 2020
@edwardmack edwardmack deleted the ed/ext_local_storage branch September 22, 2020 17:09
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