-
Notifications
You must be signed in to change notification settings - Fork 335
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
Girazoki runtime 901 moonbase alpha approval fix #974
Girazoki runtime 901 moonbase alpha approval fix #974
Conversation
* pallet-assets fixing error matching * Add erc20 instance contract * TS tests with flag to true * More typescript tests * prettier * EditorConfig * Modify integration tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks basically good. Made a comment about a few weight accounting removals to be sure they were on purpose.
gasometer.record_cost(RuntimeHelper::<Runtime>::db_read_gas_cost())?; | ||
gasometer.record_cost(RuntimeHelper::<Runtime>::db_write_gas_cost())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were wrong. I think they were left-overs from when we were using a local approval storage and not the pallet one. Now the db writes assocaited with the pallet-storage should be returned by the dispatch call itself
assetId = context.polkadotApi.createType( | ||
"AssetId", | ||
new BN("42259045809535163221576417993425387648") | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assetId could be deduced by the result of registerAsset.
Otherwise it would be more readable to declare this number as a constant: const testAssetId=new BN("42259045809535163221576417993425387648")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a fw comments on how to improve the ts test. looks great overall though!
data: data, | ||
}, | ||
]); | ||
let amount = new BN(100000000000000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better to derive amount here and in the registration from the same constant, making the test easier to modify and understand
@joelamouche can we create a JIRA-tickets for test improvements? I dont want to add to the hotfix branch something that is not in master |
Sure pls create a ticket and add details pls |
@girazoki |
Actually, let's rebase on runtime-901 and merge |
What does it do?
Provides hotfix for the approval precompile that was already merge in master.
PR that brought the fix in master: #963
What important points reviewers should know?
Is there something left for follow-up PRs?
What alternative implementations were considered?
Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
What value does it bring to the blockchain users?