Translate CW20 events into EVM events in shell receipts #1748
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -224,7 +224,7 @@ | |||
| } else { | |||
| coinbase = am.keeper.AccountKeeper().GetModuleAddress(authtypes.FeeCollectorName) | |||
| } | |||
| evmTxDeferredInfoList := am.keeper.GetEVMTxDeferredInfo(ctx) | |||
| evmTxDeferredInfoList := am.keeper.GetAllEVMTxDeferredInfo(ctx) | |||
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
codchen
force-pushed
the
use-transient-store-for-evm-deferred
branch
from
888db56 to
9902e71
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1748 +/- ##
==========================================
- Coverage 60.81% 60.79% -0.03%
==========================================
Files 373 374 +1
Lines 27116 27300 +184
==========================================
+ Hits 16491 16596 +105
- Misses 9518 9581 +63
- Partials 1107 1123 +16
|
x/evm/artifacts/README
Outdated
| @@ -1,5 +1,5 @@ | |||
| The source files are under contracts/src. The artifacts should be updated whenever the source files change. To update run the following (with NativeSeiTokensERC20 as an example): | |||
| - `solc --overwrite @openzeppelin=contracts/lib/openzeppelin-contracts --bin -o x/evm/artifacts/cw721 contracts/src/CW721ERC721Pointer.sol` | |||
| - `solc --overwrite @openzeppelin=contracts/lib/openzeppelin-contracts --bin -o x/evm/artifacts/cw20 contracts/src/CW721ERC721Pointer.sol` | |||
There was a problem hiding this comment.
should the contract also be updated to CW20ERC20Pointer.sol?
x/evm/keeper/deferred.go
Outdated
| @@ -63,3 +63,15 @@ func (k *Keeper) AppendToEvmTxDeferredInfo(ctx sdk.Context, bloom ethtypes.Bloom | |||
| } | |||
| prefix.NewStore(ctx.TransientStore(k.transientStoreKey), types.DeferredInfoPrefix).Set(key, bz) | |||
| } | |||
|
|
|||
| func (k *Keeper) GetEVMTxDeferredInfo(ctx sdk.Context) *types.DeferredInfo { | |||
There was a problem hiding this comment.
side thought: It can sometimes be nice sdk-wise to have signatures that return *pointer, bool where the bool represents found
| } | ||
| // check if there is a ERC20 pointer to contractAddr | ||
| pointerAddr, _, exists := app.EvmKeeper.GetERC20CW20Pointer(ctx, contractAddr) | ||
| if exists { |
There was a problem hiding this comment.
i'd suggest doing a if !exists { continue } to avoid the indentation
There was a problem hiding this comment.
we can't continue here because we'd have another branch for 721 https://github.com/sei-protocol/sei-chain/pull/1750/files#diff-8810d2793d992ba6de31afd91265ef3facc0612669de567b51dc1d8d029cf22fR104 I refactored this part into a func to avoid the big indentation instead
app/receipt.go
Outdated
| return amountInt.BigInt() | ||
| } | ||
| } | ||
| return big.NewInt(0) |
There was a problem hiding this comment.
is it true that the default amount should be zero (vs. nil/not-existing?). I'd also suggest returning early on !found and !ok to keep the indentations down
There was a problem hiding this comment.
(I like the GetAttributeValue approach of having the boolean to indicate found)
| if found { | ||
| seiAddr, err := sdk.AccAddressFromBech32(addrStr) | ||
| if err == nil { | ||
| evmAddr := app.EvmKeeper.GetEVMAddressOrDefault(ctx, seiAddr) |
There was a problem hiding this comment.
I believe it will, but just confirming: this would work on a past block as well (ctx's height is honored here)?
There was a problem hiding this comment.
yeah all the states are accessed via ctx which will be versioned
app/receipt.go
Outdated
| []byte(fmt.Sprintf("{\"allowance\":{\"owner\":\"%s\",\"spender\":\"%s\"}}", ownerStr, spenderStr)), | ||
| ) | ||
| if err != nil { | ||
| continue |
There was a problem hiding this comment.
is this err something that means a safe 'was not found' situation (or is it worse than that?) I wonder if it should log?
There was a problem hiding this comment.
we probably don't want a log here since it simply indicates that the contract does not adhere to cw20 (which is bad from the contract's perspective, but kind of neutral from chain's perspective)
app/receipt.go
Outdated
| } | ||
| allowanceResponse := &AllowanceResponse{} | ||
| if err := json.Unmarshal(res, allowanceResponse); err != nil { | ||
| continue |
There was a problem hiding this comment.
I think we'd want a log for this one, or maybe it's a worse situation and should bail/panic?
| topics: [ethers.id("Transfer(address,address,uint256)")] | ||
| }; | ||
| const logs = await ethers.provider.getLogs(filter); | ||
| expect(logs.length).to.equal(1); |
There was a problem hiding this comment.
it may be good just to assert the expected properties of that log (just to make sure the addresses are right, and the amounts are in the right unit)
| _ = app.EvmKeeper.SetTransientReceipt(ctx, txHash, r) | ||
| } else { | ||
| bloom = ethtypes.CreateBloom(ethtypes.Receipts{ðtypes.Receipt{Logs: logs}}) | ||
| receipt := &evmtypes.Receipt{ |
There was a problem hiding this comment.
is there some indicator or flag so that we can somehow optionally include/exclude these events from a result?
codchen
force-pushed
the
fake-receipt
branch
3 times, most recently
from
9179bc6 to
086b9c4
Compare
Describe your changes and provide context
Add a post-DeliverTx hook that captures any relevant CW20 event and translate it into a corresponding ERC20 event. If a receipt already exists for that transaction (e.g. an EVM tx that called a CW20 contract via precompile), the translated event will be appended to the existing receipt and the bloom will be updated. Otherwise, a shell receipt will be created.
Testing performed to validate your change
unit tests