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

Integrated-fixed-arwen-getCompiledCode #2430

Merged
merged 11 commits into from Nov 6, 2020

Conversation

sasurobert
Copy link
Contributor

@sasurobert sasurobert commented Nov 3, 2020

New arwen release integrated.

@@ -580,7 +580,7 @@

[VirtualMachine]
[VirtualMachine.Execution]
OutOfProcessEnabled = true
OutOfProcessEnabled = false
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be reverted later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't think so. the engine is save now to be used inprocess always

andreibancioiu
andreibancioiu previously approved these changes Nov 4, 2020
gasLimit := uint64(10000000000)

scCode := arwen.GetSCCode("../testdata/delegation/delegation_v0_5_1_full.wasm")
// 17918321 - stake in active - 11208675 staking in waiting - 28276371 - unstake from active
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this comment still apply?

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. it is for testing purposes.

andreibancioiu
andreibancioiu previously approved these changes Nov 5, 2020
andreibancioiu
andreibancioiu previously approved these changes Nov 5, 2020
return MetachainShardId, nil
}

shardID, err := strconv.ParseInt(shardIDStr, 10, 64)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ParseInt(shardIDStr, 10, 32) - but it really does not matter in this case, actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was copy paste from existing code.

compiledScStorage storage.Storer
configSCStorage config.StorageConfig
workingDir string
nilStorage bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps use the same name here, nilCompiledSCStore?

@@ -2473,7 +2484,7 @@ func CreateLatestStorageDataProvider(
) (storage.LatestStorageDataProviderHandler, error) {
directoryReader := storageFactory.NewDirectoryReader()

latestStorageDataArgs := storageFactory.ArgsLatestDataProvider{
latestStorageDataArgs := lastestData.ArgsLatestDataProvider{
Copy link
Contributor

Choose a reason for hiding this comment

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

latestData instead lastestData

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

Uint64Converter: TestUint64Converter,
BuiltInFunctions: builtInFuncs,
DataPool: tpn.DataPool,
NilCompiledSCStore: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can switch lines 1205 with 1206, as almost everywhere NilCompiledSCStore is the last argument :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Uint64Converter: &mock.Uint64ByteSliceConverterMock{},
BuiltInFunctions: builtInFunctions.NewBuiltInFunctionContainer(),
DataPool: datapool,
NilCompiledSCStore: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can switch lines 36 with 37, as almost everywhere NilCompiledSCStore is the last argument :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

compiledScStorage storage.Storer
configSCStorage config.StorageConfig
workingDir string
nilStorage bool
Copy link
Contributor

Choose a reason for hiding this comment

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

nilCompiledSCStore ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

nilStorage: args.NilCompiledSCStore,
}

err = blockChainHookImpl.makeCompiledSCStorage()
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is already called in ClearCompiledCodes whch is called below. Could be removed from 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.

No. As first you need to create. Empty it and recreate it. You want a clean storage.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@@ -544,10 +553,41 @@ func (bh *BlockChainHookImpl) ClearCompiledCodes() {
bh.compiledScPool.Clear()
err := bh.compiledScStorage.DestroyUnit()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be done first Close (line 559) and afterwards DestroyUnit ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nom destroy is the one cleaning the storage.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@@ -1,4 +1,4 @@
package factory
package lastestData
Copy link
Contributor

Choose a reason for hiding this comment

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

latestData instead lastestData. Also rename the folder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

@@ -1,4 +1,4 @@
package factory
package lastestData
Copy link
Contributor

Choose a reason for hiding this comment

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

latestData instead lastestData. Also rename the folder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

@sasurobert sasurobert merged commit 012a9ba into feat/esdt-relayed-arwen Nov 6, 2020
@sasurobert sasurobert deleted the integrated-fixed-arwen-and-tests branch November 6, 2020 09:39
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.

None yet

3 participants