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

R4R: Add new error type and script center for customized scripts feature #146

Merged
merged 5 commits into from Jul 19, 2019

Conversation

abelliumnt
Copy link
Contributor

@abelliumnt abelliumnt commented Jun 14, 2019

Add new error type for BEP12
bnb-chain/node#605

@abelliumnt abelliumnt changed the title R4R: Add new error type WIP: Add new error type Jun 19, 2019
@abelliumnt abelliumnt changed the title WIP: Add new error type R4R: Add new error type Jun 24, 2019
@abelliumnt abelliumnt force-pushed the feature/add-new-error-type branch 3 times, most recently from 79b5570 to 118cb73 Compare July 1, 2019 03:00
@abelliumnt abelliumnt changed the title R4R: Add new error type R4R: Add new error type and script center for customized scripts feature Jul 1, 2019
x/auth/script.go Outdated

type Script func(ctx sdk.Context, tx sdk.Msg) sdk.Error

var scriptsCenter = map[string][]Script{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

scriptsCenter . the name is a bit weird..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggested names?

Copy link
Collaborator

Choose a reason for hiding this comment

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

scriptsHub is better

x/auth/script.go Outdated
sdk "github.com/cosmos/cosmos-sdk/types"
)

type Script func(ctx sdk.Context, tx sdk.Msg) sdk.Error
Copy link
Contributor

Choose a reason for hiding this comment

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

if you want this Script a common script object, you should not put it here.

Copy link
Contributor

@rickyyangz rickyyangz left a comment

Choose a reason for hiding this comment

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

auth is not a good package to put the Script type.

@abelliumnt
Copy link
Contributor Author

auth is not a good package to put the Script type.

Any suggested package?

@abelliumnt abelliumnt force-pushed the feature/add-new-error-type branch 2 times, most recently from 6b27f86 to adbd17b Compare July 5, 2019 03:30
sdk.CacheMultiStore, sdk.AccountCache) {
// Get the context
ctx := getState(app, mode).Ctx.WithTxBytes(txBytes)
ctx := getState(app, mode).Ctx.WithTxBytes(txBytes).WithTx(tx)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we do not cache this Tx, as this is to tax ALL transactions for just a few of transactions among all the send.

We can still decode the Tx from txBytes when we figure out the receive address is really set the flag, which should be only a very small subset of the total transaction universe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tx is just an interface. It won't call cause too much data copy operation.
Previously, I have done an optimization as you said. But for future scripts, it's much likely they need to decode tx too. Now the implementation can provide a general solution for all future scripts. So I think it is better. @rickyyangz What do you think about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

make sense. but can we have a small test on how much it takes for a context.WithTx(tx)?

Copy link
Contributor

Choose a reason for hiding this comment

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

as discussed, let us remove the WithTxBytes as it is no use for us. cosmos itself uses it for the gas calculation which is no need for us. So we can make room for the WithTx

Copy link
Contributor

Choose a reason for hiding this comment

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

The WithTx itself is not very costly, but adding more contexts will make the context query slower as the context is a chain and query starts from the last context.
So we need to consider the order of these contexts.

x/bank/handler.go Show resolved Hide resolved
@abelliumnt
Copy link
Contributor Author

abelliumnt commented Jul 17, 2019

goos: darwin
goarch: amd64
pkg: github.com/cosmos/cosmos-sdk/types
BenchmarkContext-12          	 1000000	      1973 ns/op
BenchmarkContextWithTx-12    	 1000000	      2164 ns/op
PASS
goos: darwin
goarch: amd64
pkg: github.com/cosmos/cosmos-sdk/types
BenchmarkContext-12          	 1000000	      1964 ns/op
BenchmarkContextWithTx-12    	 1000000	      1947 ns/op
PASS

The benchmark result is not stable. But it seems that the WithTx will not result in significant performance degradation. So we don't need to concern the performance degradation.

@abelliumnt abelliumnt force-pushed the feature/add-new-error-type branch 2 times, most recently from 3710685 to af3c3e7 Compare July 18, 2019 03:20
types/context_test.go Outdated Show resolved Hide resolved
@abelliumnt abelliumnt force-pushed the feature/add-new-error-type branch 2 times, most recently from 3acb7d5 to 3f88023 Compare July 18, 2019 03:34
@darren-liu darren-liu merged commit 4ff585c into develop Jul 19, 2019
abelliumnt pushed a commit that referenced this pull request Jul 22, 2019
R4R: Add new error type and script center for customized scripts feature
@forcodedancing forcodedancing deleted the feature/add-new-error-type branch May 18, 2022 07:11
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

4 participants