-
Notifications
You must be signed in to change notification settings - Fork 60
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
rusk-wallet: Fix creation of deploy-transaction #2563
Conversation
1d48832
to
b3d053a
Compare
I do not see the actual deployment implemented in this PR, it only contains a way to generate contract id. Is there another PR which contains the deployment proper? |
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.
Nice, I was able to deploy a contract via Phoenix, 1 remark:
- why isn't the wallet checking the status of sent transaction? - it could tell the user that transaction was successful or if not, what was the error
- why does note fetching take so long? I'd compare with how it is done in deployment tool as it seems to be much faster there
- status feedback during deployment is flashing messages too quickly, it is not readable, and it is showing msg about streaming when it is actually doing proving, it'd be better to show more static messages about what the wallet is actually doing
@HDauven also pointed to me to this. He was going to add an issue |
Sorry @miloszm @moCello, had to participate in a long meeting before I was able to finalize the issue. Here it is: #2600 |
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.
I am still not happy with the const names for the default gas values.
The const for gas-price now named DEFAULT_PRICE_TRANSACTION
suggests that this value should be used everywhere where we also use DEFAULT_LIMIT_TRANSACTION
and wise versa. But the value for DEFAULT_LIMIT_TRANSACTION
is only used in transfer-transactions whereas the DEFAULT_PRICE_TRANSACTION
is used for all transactions.
The context in which these two constants are used is different and their name needs to reflect that.
cce88bc
to
c883734
Compare
- Change the default gas limits - Let the wallet use the deftault values for deployment - Rename gas constants - Fix merge conflicts - Rename nonce variable to seperate from deploy_nonce for moonlight
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.
LGTM
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.
LGTM
This pick up the changes to the deploy-transaction creation from #2540 after it has been reverted and separated from the changes made to the syncing (#2558)