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

Regression in CreateStorageKey #168

Closed
vgeddes opened this issue Jun 18, 2021 · 12 comments
Closed

Regression in CreateStorageKey #168

vgeddes opened this issue Jun 18, 2021 · 12 comments

Comments

@vgeddes
Copy link
Contributor

vgeddes commented Jun 18, 2021

We are users of GSRPC (and contributors too) in https://github.com/Snowfork/polkadot-ethereum. The new API for CreateStorageKey breaks compatibility. But it also includes a regression:

Suppose I have a single storage item 'MyPallet.Nonce'. It is a not a map or a double-map. However calling types.CreateStorageKey(metadata, "MyPallet", "Nonce") fails with:

Nonce is a map, therefore requires precisely one argument. received: 0

So to get it to work I have to pass nil like this, which is confusing, since Nonce is very certainly not a map:

types.CreateStorageKey(metadata, "MyPallet", "Nonce", nil)
@ThreeAndTwo
Copy link

ThreeAndTwo commented Jun 18, 2021

@vedhavyas
same error. those demos exist error in gsrpc-go-dev-demo, because v13 for CreateStorageKey breaks compatibility. types.CreateStorageKey(meta, "System", "Account", account.PublicKey, nil)
error info:

Account is a map, therefore requires precisely one argument. received: 2

@vedhavyas
Copy link
Contributor

I'm reverting the breaking change, keeping the metadata v13 make a release in a bit. Thank you!

@vedhavyas
Copy link
Contributor

Sorry about this 🙏🏼

@ParthDesai
Copy link
Contributor

ParthDesai commented Jun 21, 2021

@vgeddes @vedhavyas I have fixed CreateStorageKey to be backward compatible.
And also fixed that regression as well.

PR to make it backward compatible: #166

@ParthDesai
Copy link
Contributor

ParthDesai commented Jun 21, 2021

@vgeddes @vedhavyas @ThreeAndTwo PR: #169 adds comprehensive test cases for CreateStorageKey.

@vedhavyas
Copy link
Contributor

@vgeddes @ThreeAndTwo can you confirm if the regression is fixed with the PR above ?

@ParthDesai
Copy link
Contributor

@vgeddes @ThreeAndTwo can you confirm if the regression is fixed with the PR above ?

@vedhavyas The regression was already fixed via PR: #166
But, it was not covered by tests. PR: #169 aims to cover the new changes made in CreateStorageKey with tests.

@ThreeAndTwo
Copy link

@vedhavyas @ParthDesai I tested it. CreateStorageKey is normal, but I find decimal have inaccurate for token.
current balance: 855.359271
before balance: 8.55359271
in fact balance: 8.55359271

@ParthDesai
Copy link
Contributor

@vedhavyas @ParthDesai I tested it. CreateStorageKey is normal, but I find decimal have inaccurate for token.
current balance: 855.359271
before balance: 8.55359271
in fact balance: 8.55359271

This may not be related to CreateStorageKey at all.

@vedhavyas
Copy link
Contributor

This may not be related to CreateStorageKey at all.

True.

@ThreeAndTwo double check your type please. Its usually the case

@vedhavyas
Copy link
Contributor

closing this since its confirmed to be fixed in latest patch.

@ThreeAndTwo please open an new issue if the above mentioned decimal issue still exists.

@ThreeAndTwo
Copy link

yeah, everything is ok, thx.

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

No branches or pull requests

4 participants