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

9212 chain helpers #9439

Merged
merged 14 commits into from
Jun 4, 2024
Merged

9212 chain helpers #9439

merged 14 commits into from
Jun 4, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented May 31, 2024

refs: #9212

Description

Incremental progress on #9212

Plus some drive-by improvements

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

@turadg turadg requested review from dckc and 0xpatrickdev May 31, 2024 23:36
Copy link

cloudflare-workers-and-pages bot commented May 31, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 19bc874
Status: ✅  Deploy successful!
Preview URL: https://dac32715.agoric-sdk.pages.dev
Branch Preview URL: https://9212-chain-helpers.agoric-sdk.pages.dev

View logs

@turadg turadg mentioned this pull request May 31, 2024
@turadg turadg changed the base branch from ta/bridge-types to ta/network-tools June 3, 2024 23:31
@turadg turadg marked this pull request as ready for review June 3, 2024 23:31
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

makeAccount() in the facade should return a local object

Base automatically changed from ta/network-tools to master June 4, 2024 15:06
mergify bot added a commit that referenced this pull request Jun 4, 2024
refs: #9212

## Description

#9439 needs some of the fakes
in the network tests. This wraps them up a bit for re-use elsewhere.

### Security Considerations
n/a, test code
<!-- Does this change introduce new assumptions or dependencies that, if
violated, could introduce security vulnerabilities? How does this PR
change the boundaries between mutually-suspicious components? What new
authorities are introduced by this change, perhaps by new API calls?
-->

### Scaling Considerations
n/a, test code

<!-- Does this change require or encourage significant increase in
consumption of CPU cycles, RAM, on-chain storage, message exchanges, or
other scarce resources? If so, can that be prevented or mitigated? -->

### Documentation Considerations

API docs may show these

### Testing Considerations

per se

### Upgrade Considerations

n/a, test code
@turadg turadg added the force:integration Force integration tests to run on PR label Jun 4, 2024
@turadg turadg requested a review from dckc June 4, 2024 17:28
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

I'm a little fuzzy on how all the pieces relate, but I suppose they're OK.

},
});

// TODO find a way to wait until completionTime, so we can check the count has gone back down
Copy link
Member

Choose a reason for hiding this comment

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

We can query and then wait like we do for blocks etc.

It's 21 days, so we probably want to reduce it to something like 15sec first.

$ agd query staking params -o json | jq  .unbonding_time
"1814400s"

const addressStr = await E(account).getAddress();
return {
address: addressStr,
chainId: 'agoric-3',
Copy link
Member

Choose a reason for hiding this comment

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

chainId will be agoriclocal in a3p, right? seems like chainId should be passed in. Or a TODO or something.

Comment on lines +64 to +70
send(toAccount, amount) {
// FIXME implement
console.log('send got', toAccount, amount);
},
Copy link
Member

Choose a reason for hiding this comment

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

local-chain-account-kit implements send(), right? are we not ready to wire it up?

Copy link
Member Author

Choose a reason for hiding this comment

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

To know it's covered I've been leaning to writing a breaking test first and then implementing until green

Comment on lines +64 to +65
await t.throwsAsync(E(account).getBalances(), {
message: 'not yet implemented',
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this has an implicit TODO

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Jun 4, 2024
Comment on lines 141 to 143
console.error(
'FIXME deposit noop until https://github.com/Agoric/agoric-sdk/issues/9193',
);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we'll have deposit or getPurse with 9193. The plan is to demonstrate how to use a LocalChainAccount to deposit (and transfer) ERTP assets to a ChainAccount.

portAllocator,
protocol,
when,
};
Copy link
Member

Choose a reason for hiding this comment

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

🙌

@mergify mergify bot merged commit ceec1d5 into master Jun 4, 2024
63 checks passed
@mergify mergify bot deleted the 9212-chain-helpers branch June 4, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants