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

make GetBalance return the sum of native & sgt balance #24

Open
wants to merge 1 commit into
base: op-es
Choose a base branch
from

Conversation

blockchaindevsh
Copy link
Collaborator

This is a workaround for ethstorage/optimism#148 .

@@ -688,8 +688,8 @@ func (api *BlockChainAPI) GetBalance(ctx context.Context, address common.Address
if state == nil || err != nil {
return nil, err
}
b := state.GetBalance(address).ToBig()
return (*hexutil.Big)(b), state.Error()
nativeBalance, sgtBalance := core.GetGasBalancesInBig(state, api.b.ChainConfig(), address)
Copy link

Choose a reason for hiding this comment

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

Should we use a flag to turn it on or off? Or another idea is to use a flag with different port number for SGT balance RPC

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What about re-using OptimismConfig.UseSoulGasToken as the flag to enable this change?

@qzhodl
Copy link

qzhodl commented Dec 30, 2024

Has the workaround been tested with MetaMask to confirm it works?

@blockchaindevsh
Copy link
Collaborator Author

blockchaindevsh commented Dec 30, 2024

Has the workaround been tested with MetaMask to confirm it works?

Not yet, but I've shipped it to the beta testnet since it doesn't affect consensus. Could you have another try ? If not , I can do it a bit later.

@qzhodl
Copy link

qzhodl commented Dec 30, 2024

Has the workaround been tested with MetaMask to confirm it works?

Not yet, but I've shipped it to the beta testnet since it doesn't affect consensus. Could you have another try ? If not , I can do it a bit later.

It worked on my side. However, as we discussed earlier, the side effect is that Metamask displays the SGT balance, which might confuse users when they attempt to transfer it, as the transaction will ultimately fail.

@blockchaindevsh
Copy link
Collaborator Author

blockchaindevsh commented Dec 30, 2024

Oops, looks like if user specifies a msg.value greater than native balance, metamask won't stop the user from sending the tx, and the tx will fail like this:

image

But the failed tx isn't on chain, as can be verified by checking nonce.

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.

3 participants